Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP+RFC add chunked mode #570

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

WIP+RFC add chunked mode #570

wants to merge 10 commits into from

Conversation

oscardssmith
Copy link
Member

based on discussion JuliaDiff/Diffractor.jl#54 and in slack. This is an initial implementation of forward and reverse chunked mode AD.

src/config.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
oscardssmith and others added 7 commits July 26, 2022 17:15
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
oscardssmith and others added 2 commits July 27, 2022 12:55
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -7,7 +7,7 @@ using Compat: hasfield, hasproperty

export frule, rrule # core function
# rule configurations
export RuleConfig, HasReverseMode, NoReverseMode, HasForwardsMode, NoForwardsMode
export RuleConfig, HasReverseMode, NoReverseMode, HasForwardsMode, NoForwardsMode, HasChunkedMode, NoChunkedMode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
export RuleConfig, HasReverseMode, NoReverseMode, HasForwardsMode, NoForwardsMode, HasChunkedMode, NoChunkedMode
export RuleConfig,
HasReverseMode,
NoReverseMode,
HasForwardsMode,
NoForwardsMode,
HasChunkedMode,
NoChunkedMode

Comment on lines +89 to +90
function frule(::RuleConfig{>:HasChunkedMode},
(Δf, Δx)::Tuple{Any,ProductTangent}, f, args...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
function frule(::RuleConfig{>:HasChunkedMode},
(Δf, Δx)::Tuple{Any,ProductTangent}, f, args...)
function frule(
::RuleConfig{>:HasChunkedMode}, (Δf, Δx)::Tuple{Any,ProductTangent}, f, args...
)

src/rules.jl Show resolved Hide resolved
Comment on lines +96 to +99
function rrule(::RuleConfig{>:HasChunkedMode}, args...)
y, back = rrule(args...)
return y, ApplyBack(back)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I suggested this RuleConfig idea, but I'm not sure it fits. If there is an un-chunked rrule which needs the configuration, then this rrule(args...) can't call it.

I think it might be cleanest to have a new function, whose fallback is chunked_rrule(config, f, args...) = rrule(config, f, args...). Any AD which may make chunks should always call this, and any AD rule to exploit chunks will overload it.

Not sure how that intersects with @opt_out. Maybe there are other weird corners to think about too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. We might need it to be separate. It feels clunky, but this method also doesn't seem great.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ideally we want to hit the version with this particular trait dropped, and other traits preserved (like HasReverseMode etc)
We should be able to achieve that with invoke, but can we do it elegantly?

Comment on lines +81 to +83
struct ProductTangent{P}
partials::P
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much is gained from this kind of chunking? I mean storing several partials in some tuple. For reverse mode, nothing, I think. For forward mode, maybe derivatives_using_output has the same benefits.

The big gain seems to be from making a solid matrix to represent many vectors, and e.g. getting matrix multiplication. So I think the initial design ought to make that work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This design does this (I think). partials here can either be a Tuple{Tuple} or an eachrow(Matrix)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I guess a vector of vectors can have the same meaning as an EachCol. Just a different path, by dispatch on ProductTangent{<:EachSlice}.

@oxinabox oxinabox added the speculative fairly out there ideas for consideration in longer term label Aug 5, 2022
@oxinabox oxinabox changed the title add chunked mode WIP+RFC add chunked mode Aug 5, 2022
@oxinabox
Copy link
Member

oxinabox commented Aug 5, 2022

Having some tests would prove illuminating I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
speculative fairly out there ideas for consideration in longer term
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants