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

Add Symbol -> AbstractADType mapping #62

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Add Symbol -> AbstractADType mapping #62

merged 4 commits into from
Jun 17, 2024

Conversation

gdalle
Copy link
Collaborator

@gdalle gdalle commented Jun 17, 2024

In gdalle/DifferentiationInterface.jl#319, @MilesCranmer proposed a mapping from package Symbols to AbstractADTypes. 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 Symbols, since ADTypes.jl is precisely there to avoid their shortcomings. Hence the strongly-worded docstring.

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

@ChrisRackauckas
Copy link
Member

I'm at least not opposed with the proper warnings on it.

@gdalle
Copy link
Collaborator Author

gdalle commented Jun 17, 2024

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.

@ChrisRackauckas
Copy link
Member

That's probably sufficient.

Copy link

@MilesCranmer MilesCranmer left a 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

src/symbols.jl Outdated Show resolved Hide resolved
src/symbols.jl Outdated Show resolved Hide resolved
src/symbols.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Co-authored-by: Vaibhav Kumar Dixit <vaibhavyashdixit@gmail.com>
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
src/symbols.jl Outdated Show resolved Hide resolved
test/symbols.jl Outdated Show resolved Hide resolved
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
@MilesCranmer
Copy link

Looks pretty good to me 👍

@gdalle
Copy link
Collaborator Author

gdalle commented Jun 17, 2024

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?

@MilesCranmer
Copy link

MilesCranmer commented Jun 17, 2024

Cool, confirmed it works! :)

Screenshot 2024-06-17 at 18 57 17

and, in the Options constructor (type unstable on purpose; it's just an entrypoint function that the user will then pass to the actual search)

Screenshot 2024-06-17 at 19 00 25

@gdalle gdalle marked this pull request as ready for review June 17, 2024 18:33
Copy link
Member

@Vaibhavdixit02 Vaibhavdixit02 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gdalle gdalle merged commit 72f806d into main Jun 17, 2024
4 checks passed
@MilesCranmer
Copy link

Thanks!

(Registering this soon would be great if possible 😁 I'm eager to put this into SymbolicRegression.jl/PySR)

@gdalle
Copy link
Collaborator Author

gdalle commented Jun 18, 2024

done!

@avik-pal avik-pal deleted the gd/symbols branch June 22, 2024 19:33
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.

None yet

4 participants