-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add Symbol -> AbstractADType mapping #62
Conversation
I'm at least not opposed with the proper warnings on it. |
I worded the docstring as follows, let me know if we should make it stronger. I also ensured it is displayed at the very bottom of the API reference page. ADTypes.Auto(package::Symbol)
A shortcut that converts an AD package name into an instance of [`AbstractADType`](@ref),
with all parameters set to their default values.
!!! warning
This function is type-unstable by design and might lead to suboptimal performance.
In most cases, you should never need it: use the individual backend types directly. |
That's probably sufficient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, here are some suggestions to simplify and make it possible for me to provide a ad_backend_options
parameter
Co-authored-by: Vaibhav Kumar Dixit <[email protected]> Co-authored-by: Miles Cranmer <[email protected]>
Co-authored-by: Miles Cranmer <[email protected]>
Looks pretty good to me 👍 |
Can you try it out in your SymbolicRegression.jl use case before we merge, to be sure we didn't miss something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks! (Registering this soon would be great if possible 😁 I'm eager to put this into SymbolicRegression.jl/PySR) |
done! |
In gdalle/DifferentiationInterface.jl#319, @MilesCranmer proposed a mapping from package
Symbol
s toAbstractADType
s. While this mapping is not strictly necessary, it makes things easier for interfacing with Python. Here's an experimental PR doing just that, let's see what people think.Warning
I don't want to encourage the use of
Symbol
s, since ADTypes.jl is precisely there to avoid their shortcomings. Hence the strongly-worded docstring.Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.