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 AutoMooncake type #89

Merged
merged 13 commits into from
Sep 25, 2024
Merged

Add AutoMooncake type #89

merged 13 commits into from
Sep 25, 2024

Conversation

willtebbutt
Copy link
Contributor

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

Additional context

AutoTapir needs to be deprecated in favour of AutoMooncake, because Tapir.jl has been renamed to Mooncake.jl.

I could do with some advice on the appropriate way to deprecate AutoTapir -- should I just add the constructor to the legacy.jl file, or is there something else that I should do?

@ChrisRackauckas
Copy link
Member

I could do with some advice on the appropriate way to deprecate AutoTapir -- should I just add the constructor to the legacy.jl file, or is there something else that I should do?

@deprecate AutoTapir AutoMoonCake?

@gdalle
Copy link
Collaborator

gdalle commented Sep 25, 2024

Can we have a default setting so that people don't need to dig into the Mooncake.Config docs? Like AutoMooncake(; config=nothing)?

@willtebbutt
Copy link
Contributor Author

We could do that. I'd prefer to prevent people from writing AutoMooncake() -- they should at least have to write AutoMooncake(; config=nothing) -- in order to ensure that people are aware that they have some configuration options.

@gdalle
Copy link
Collaborator

gdalle commented Sep 25, 2024

Sounds good, let's do that. As a user of ADTypes it's always a bit painful to go grab a specific type from a package to configure backends (like AutoChainRules(; config=Zygote.ZygoteRuleConfig()))

@gdalle gdalle self-requested a review September 25, 2024 11:00
@gdalle gdalle self-assigned this Sep 25, 2024
@gdalle
Copy link
Collaborator

gdalle commented Sep 25, 2024

Ready for review?

@willtebbutt
Copy link
Contributor Author

Yes, go for it.

Copy link
Collaborator

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Most of my remarks are esthetics, fundamentally it is good to go

docs/src/index.md Outdated Show resolved Hide resolved
src/dense.jl Outdated Show resolved Hide resolved
src/dense.jl Outdated Show resolved Hide resolved
src/dense.jl Outdated Show resolved Hide resolved
src/dense.jl Outdated Show resolved Hide resolved
src/legacy.jl Outdated Show resolved Hide resolved
test/legacy.jl Show resolved Hide resolved
@gdalle gdalle merged commit c958378 into SciML:main Sep 25, 2024
5 checks passed
@willtebbutt willtebbutt deleted the wct/automooncake branch September 25, 2024 11:49
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