SciML / ADTypes.jl

Repository for automatic differentiation backend types
https://sciml.github.io/ADTypes.jl/
MIT License
36 stars 11 forks source link

Fix constructor of AutoMTK #61

Closed Vaibhavdixit02 closed 1 month ago

Vaibhavdixit02 commented 1 month ago

Checklist

Additional context

I was overloading the constructor in OptimizationBase since the deprecation here wasn't sufficient, leading to re-precompilation. I couldn't find a way to use @deprecated for this so this'll have to do.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (733a26a) to head (7cde295).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #61 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 6 7 +1 Lines 65 75 +10 ========================================= + Hits 65 75 +10 ``` | [Flag](https://app.codecov.io/gh/SciML/ADTypes.jl/pull/61/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SciML) | Coverage Δ | | |---|---|---| | [docs](https://app.codecov.io/gh/SciML/ADTypes.jl/pull/61/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SciML) | `2.04% <0.00%> (-0.53%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SciML#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gdalle commented 1 month ago

I'll review tomorrow, please wait for me

Vaibhavdixit02 commented 1 month ago

Sure

Vaibhavdixit02 commented 1 month ago

Thanks, I wasn't completely sure what you meant by it in the chat

Vaibhavdixit02 commented 1 month ago

I'll wait for @gdalle to take a look to make sure it doesn't affect DI

gdalle commented 1 month ago

The AutoModelingToolkit struct in ADTypes v0.2.7 was as follows, so this is the full API we want to deprecate but with meaningful fallbacks: https://github.com/SciML/ADTypes.jl/blob/9f17b8227afdb12cbeb0faa7edc18abd5ffcae0c/src/ADTypes.jl#L151-L164 @ChrisRackauckas is right that the deprecation logic wasn't correct, so my last commit fixes it. We still generate a deprecation warning, which will be visible to users if such warnings are enabled, but we fall back on the correct version of the Symbolics backend. @Vaibhavdixit02 what do you think? I took cues from https://invenia.github.io/blog/2022/06/17/deprecating-in-julia/

ChrisRackauckas commented 1 month ago

Why did you change it back to being incorrect? The point is that there was a version without kwargs before.

gdalle commented 1 month ago

My fallbacks handle every version that there was before, with and without kwargs. Both will give the correct result, but both will be deprecated. The old struct had a Base.@kwdef and the keywords had default values, but I also take care of the standard struct constructor with positional arguments and no default values.

ChrisRackauckas commented 1 month ago

I didn't see the other dispatch. This should work then.

gdalle commented 1 month ago

Let's wait for Vaibhav to test it locally with OptimizationBase before merging