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

test: remove unnecessarily specific parameter type annotations #1010

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

AayushSabharwal
Copy link
Member

With SciML/ModelingToolkit.jl#2908, tunables are being merged into one Vector{T}, where T is some floating point number. This is because tunables are intended for optimization and sensitivity analysis, but AD doesn't work for integers. Not splitting into Tuple{Vector{T1}, Vector{T2}} also has performance benefits for sensitivity analysis, and helps reduce a significant number of bugs. Array tunables are also concatenated into this vector. Accessing these functions is done through a view to prevent unnecessary performance overhead, even in MTK's generated functions.

To this end, parameters are promoted to a common type. Overly specific type annotations are also in general invalid, since the parameters could also be duals.

This PR is a draft so it can be rerun once to aforementioned MTK PR is merged and tagged.

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

Add any other context about the problem here.

@ChrisRackauckas
Copy link
Member

Not splitting into Tuple{Vector{T1}, Vector{T2}} also has performance benefits for sensitivity analysis, and helps reduce a significant number of bugs. Array tunables are also concatenated into this vector. Accessing these functions is done through a view to prevent unnecessary performance overhead, even in MTK's generated functions.

And not to mention, no AD or optimization library supports having the tuning vector be different types, and I don't see that changing any time soon, and I don't see a case where that's a real optimization.

@isaacsas
Copy link
Member

isaacsas commented Aug 2, 2024

Thanks for helping with any migration this will entail. Would this then mean we can’t have integer parameters that stay integer or is that not an issue as they aren’t tunable?

Also, isn’t this a breaking change at some level? I’d imagine type specific user registered functions may stop working if parameters are being silently promoted now. Will this be held for a MTK 10 release?

@isaacsas
Copy link
Member

isaacsas commented Aug 2, 2024

Note there are likely doc updates needed here too as I believe @TorkelE wrote about typing parameters for the V14 docs too.

@ChrisRackauckas
Copy link
Member

It's not breaking if integer-valued parameters are not marked as tunable. This is just changing the internal representation of tuanbles. @AayushSabharwal confirm that @parameters p=1 has that default to non-tunable? Only AbstractFloats should default to tuanble.

@AayushSabharwal
Copy link
Member Author

Thanks for helping with any migration this will entail. Would this then mean we can’t have integer parameters that stay integer or is that not an issue as they aren’t tunable?

They will still be integers, they just won't be in the tunables portion. @parameters p::Integer = 1 will still be stored as an integer.

@parameters p=1 has that default to non-tunable? Only AbstractFloats should default to tuanble.

Only AbstractFloats would default to tunable yes, but @parameters p = 1 is by default a BasicSymbolic{Real} which we treat as a float. To get an integer, you need the type annotation.

@ChrisRackauckas
Copy link
Member

That would be breaking then, because we previously respected it as an integer there. I really think we should set tunable more carefully by default. I don't think we can go forward with this without it, since otherwise you very easily get rid of people's integers and bools.

@AayushSabharwal
Copy link
Member Author

It's not breaking. @parameters p = 1 was always a float, since v9

@AayushSabharwal
Copy link
Member Author

Parameters that users have explicitly marked as integers/bools will still be integers/bools, just in a different partition.

@ChrisRackauckas
Copy link
Member

Oh, I thought we had a conversion. Okay, if that was already respecting the symtype then we're good.

@isaacsas
Copy link
Member

isaacsas commented Aug 2, 2024

We already assume that if a symbolic parameter isn’t explicitly annotated as an integer symbolic it gets converted so that case shouldn’t be an issue.

@ChrisRackauckas ChrisRackauckas marked this pull request as ready for review August 2, 2024 15:02
@ChrisRackauckas ChrisRackauckas merged commit 1e9e216 into SciML:master Aug 2, 2024
4 checks passed
@TorkelE
Copy link
Member

TorkelE commented Aug 3, 2024

Did these additionally specific typings cause error in tests with new MTK? The tests here was not really intended to be optimised code for AD, but rather to test various really niche cases and that these works, to potentially catch niche errors.

@AayushSabharwal AayushSabharwal deleted the as/remove-bad-test branch August 4, 2024 14:11
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.

4 participants