-
-
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
Fix constructor of AutoMTK #61
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 7 +1
Lines 65 75 +10
=========================================
+ Hits 65 75 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I'll review tomorrow, please wait for me |
Sure |
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.
Just needs the maxlog thing and this is good.
Co-authored-by: Christopher Rackauckas <accounts@chrisrackauckas.com>
Thanks, I wasn't completely sure what you meant by it in the chat |
I'll wait for @gdalle to take a look to make sure it doesn't affect DI |
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.
I don't see the point of introducing a new object when it's already deprecated, and it doesn't correspond to an actual autodiff backend.
We made a breaking release of ADTypes, a breaking release of OptimizationBase might be coming, we should leverage that instead of trying to minimize breakage by sticking to old patterns.
If you need a shortcut for Symbolics with or without sparsity in OptimizationBase, why not use AutoModelingToolkitDeprecated
or something of that sort, which you define yourself?
The Lines 151 to 164 in 9f17b82
@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/ |
Why did you change it back to being incorrect? The point is that there was a version without kwargs before. |
My fallbacks handle every version that there was before, with and without kwargs. Both will give the correct result, but both will be deprecated. |
I didn't see the other dispatch. This should work then. |
Let's wait for Vaibhav to test it locally with OptimizationBase before merging |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
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.