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

Fix constructor of AutoMTK #61

Merged
merged 4 commits into from
Jun 7, 2024
Merged

Fix constructor of AutoMTK #61

merged 4 commits into from
Jun 7, 2024

Conversation

Vaibhavdixit02
Copy link
Member

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

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.

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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     
Flag Coverage Δ
docs 2.04% <0.00%> (-0.53%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gdalle
Copy link
Collaborator

gdalle commented Jun 6, 2024

I'll review tomorrow, please wait for me

@Vaibhavdixit02
Copy link
Member Author

Sure

src/legacy.jl Outdated Show resolved Hide resolved
Copy link
Member

@ChrisRackauckas ChrisRackauckas left a 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>
@Vaibhavdixit02
Copy link
Member Author

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

@Vaibhavdixit02
Copy link
Member Author

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

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.

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?

test/legacy.jl Show resolved Hide resolved
@gdalle
Copy link
Collaborator

gdalle commented Jun 7, 2024

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:

ADTypes.jl/src/ADTypes.jl

Lines 151 to 164 in 9f17b82

"""
AutoModelingToolkit
Chooses [ModelingToolkit.jl](https://github.com/SciML/ModelingToolkit.jl).
# Fields
- `obj_sparse::Bool = false`
- `cons_sparse::Bool = false`
"""
Base.@kwdef struct AutoModelingToolkit <: AbstractSymbolicDifferentiationMode
obj_sparse::Bool = false
cons_sparse::Bool = false
end

@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
Copy link
Member

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

@gdalle
Copy link
Collaborator

gdalle commented Jun 7, 2024

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
Copy link
Member

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

@gdalle
Copy link
Collaborator

gdalle commented Jun 7, 2024

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

@ChrisRackauckas ChrisRackauckas merged commit 6a76e11 into main Jun 7, 2024
6 checks passed
@ChrisRackauckas ChrisRackauckas deleted the mtk branch June 7, 2024 12:18
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

3 participants