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

Document Enzyme.Compiler.recursive_accumulate #1894

Closed
wants to merge 1 commit into from

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Sep 26, 2024

Adds a docstring for Enzyme.Compiler.recursive_accumulate and displays that docstring in the API reference.
I assume we should do the same for recursive_add but I don't exactly understand what it does.

This is needed in DifferentiationInterface for accumulating into the shadow between forward and reverse pass in autodiff_thunk.

@wsmoses
Copy link
Member

wsmoses commented Sep 26, 2024

This is very much an internal function subject to change at any point.

@gdalle
Copy link
Contributor Author

gdalle commented Sep 26, 2024

Is there an alternative that might be more flexible?
I'm not asking for behavior to be guaranteed, I'm just wondering what to use in lieu of .+= when I accumulate a dresult here:
https://github.com/gdalle/DifferentiationInterface.jl/blob/b93c17e918fef763a62bcc73d5b9021dc906e505/DifferentiationInterface/ext/DifferentiationInterfaceEnzymeExt/reverse_onearg.jl#L14

@wsmoses
Copy link
Member

wsmoses commented Sep 26, 2024

not offhand (which you're welcome to use this right now with the forewarning that this is an internal function whose semantics are subject to change). Thus a patch may chance its semantics and/or arguments (e.g. per #1852)

@gdalle
Copy link
Contributor Author

gdalle commented Sep 26, 2024

But regardless of the changes in semantics, it will always be the precise function I need to recursively accumulate into a shadow, right?

@wsmoses
Copy link
Member

wsmoses commented Sep 26, 2024

....kind of. At the moment the semantics are such that it does y .+= f(x) for the non mutable internal states.

e.g. presently this works for a Vector{Float64} and Vector{Tuple{Float64, Float64}} however it will not recursively add the inner types of Vector{Vector{Float64}}

@danielwe
Copy link
Contributor

PSA I'm working on improving recursive_add and recursive_accumulate here: #1852. This will remove the limitations @wsmoses mentioned, and if you want to have it, provide general functions recursive_map and recursive_map! that can do more things than just add/accumulate into the first arg (basically anything involving commensurate iteration over one or more objects with identical structure, including being the basis for make_zero). This is also necessary to write custom rules for higher-order functions. Will let you know when ready for feedback and discussion about this. It won't be long.

@danielwe
Copy link
Contributor

I also think these functions should be internal and that the public interface for rule writing and DI's use cases should be a VectorSpace wrapper that you can wrap around any object, that overloads the necessary operations like + and * and uses these functions internally. A prototype of this currently lives in QuadGK's Enzyme extension, but if @wsmoses is OK with it I think it would be natural to add this to EnzymeRules once it's mature.

@danielwe
Copy link
Contributor

Lol, didn't see that @wsmoses already linked to the PR, oh well, hope you enjoyed the elevator pitch

@wsmoses
Copy link
Member

wsmoses commented Sep 27, 2024

Yeah I concur @danielwe .

The only reason that QuadGK gets away with using this atm is the fact that I wrote that extension and commit to maintaining it (as we migrate to a public style interface)

@gdalle
Copy link
Contributor Author

gdalle commented Sep 27, 2024

@danielwe I actually did enjoy the elevator pitch, and I would love a public API that behaves like a vector space. Feel free to ping me when this emerges!

@wsmoses
Copy link
Member

wsmoses commented Oct 23, 2024

Closing this per above discussion

@wsmoses wsmoses closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants