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

What is the right way to specify the Enzyme mode? #24

Closed
gdalle opened this issue Mar 8, 2024 · 10 comments · Fixed by #40
Closed

What is the right way to specify the Enzyme mode? #24

gdalle opened this issue Mar 8, 2024 · 10 comments · Fixed by #40
Labels
question Further information is requested

Comments

@gdalle
Copy link
Collaborator

gdalle commented Mar 8, 2024

Question❓

The source file says nothing, the test file recommends something but does something else:

# In practice, you would rather specify a
# `mode::Enzyme.Mode`, e.g. `Enzyme.Reverse` or `Enzyme.Forward`
adtype = AutoEnzyme(; mode = Val(:Reverse))
@test adtype isa ADTypes.AbstractADType
@test adtype isa AutoEnzyme{Val{:Reverse}}

What convention is used in the SciML ecosystem? Can it be documented?

@gdalle gdalle added the question Further information is requested label Mar 8, 2024
@Vaibhavdixit02
Copy link
Member

I haven't been using the mode kwarg, idk if @avik-pal uses it. For me in Optimization.jl it is just reverse mode for gradient and forward over reverse for hessian so not well suited for the mode argument.

@avik-pal
Copy link
Member

avik-pal commented Mar 8, 2024

Me neither I kind of always assume it's reverse mode (which I probably shouldn't)

@gdalle
Copy link
Collaborator Author

gdalle commented Mar 13, 2024

Thanks! Since you're maintaining packages that make use of ADTypes.jl, I'd be glad to have your opinion on https://github.com/gdalle/DifferentiationInterface.jl. The gist is "AbstractDifferentiation.jl + ADTypes.jl"

@gdalle
Copy link
Collaborator Author

gdalle commented Mar 14, 2024

Another question is what to put inside the AutoEnzyme object, beyond the "mode". Do we need more information to make informed decisions?

@wsmoses
Copy link
Collaborator

wsmoses commented Mar 14, 2024

I feel that here it would actually be better for ADTypes to just re export enzyme.reverse/etc when packages have these (they are defined in enzymecore which contains no dependencies and just define enzyme interfaces).

I think the fact that this is optional is a rather important misnomer. You wouldn't expect autoforwarddiff to call reverse mode.jl

Also just like forward mode has chunking enzyme does as well so there's an ambiguous question of what chunk type you're doing.****

@Vaibhavdixit02
Copy link
Member

Since you're maintaining packages that make use of ADTypes.jl, I'd be glad to have your opinion on https://github.com/gdalle/DifferentiationInterface.jl.

It looks great! Some other features already being discussed in JuliaDiff/DifferentiationInterface.jl#26, caching etc and support for sparsity will also be pretty essential for me to consider using it in Optimization.jl

@avik-pal
Copy link
Member

I think the fact that this is optional is a rather important misnomer

I feel nothing is not a bad default. Whenever we keep a field as nothing (at least in NonlinearSolve and such repos), the assumption there is use the best possible heuristic. Let's say you have a nested situation then we can do forward over reverse, for a small system we do a forward, and so on...

Having said that the current form does make it hard to subtype it as forward / reverse mode. I think we should split it up into AutoForwardEnzyme / AutoReverseEnzyme. My main issue with reexporting EnzymeCore is not being able to subtype them as Abstract(Reverse/Forward)

@gdalle
Copy link
Collaborator Author

gdalle commented Mar 15, 2024

@avik-pal There's also another issue with using Enzyme.Reverse as mode, which is that depending on the function (like "value_and_gradient") you may want Enzyme.ReverseWithPrimal instead

@gdalle
Copy link
Collaborator Author

gdalle commented Mar 15, 2024

@Vaibhavdixit02 indeed! Cache support is fully there for ForwardDiff and ReverseDiff (with and without compiled tape) already, with a unified interface. FiniteDiff and SparseDiffTools are next on my hit list

@avik-pal
Copy link
Member

There's also another issue with using Enzyme.Reverse as mode, which is that depending on the function (like "value_and_gradient") you may want Enzyme.ReverseWithPrimal instead

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants