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

Make default value units consistent #2898

Merged
merged 12 commits into from
Aug 7, 2024

Conversation

contradict
Copy link
Contributor

@contradict contradict commented Jul 26, 2024

When using units in a component, it would be nice if those units could be checked on the default and initial values. With old ModelingToolkit, it was possible to do @parameters p = ustrip(expected_unit, provided_value), [unit=expected_unit], but that is not longer possible. This PR is a draft of a way to restore that functionality. It only works with Unitful and only with scalar variables, but these could both be fixed if this is desirable.

The result is a component defined like this

julia> @mtkmodel ATest begin
       @parameters begin
       some=1.0, [description="Some", unit=u"kg"]
       more, [description="more", unit=u"L"]
       end
       end
ModelingToolkit.Model{typeof(__ATest__), Dict{Symbol, Any}}(__ATest__, Dict{Symbol, Any}(:kwargs => Dict{Symbol, Dict}(:some => Dict{Symbol, Any}(:value => 1.0, :type => Real), :more => Dict{Symbol, Union{Nothing, DataType}}(:value => nothing, :type => Real)), :independent_variable => t, :parameters => Dict{Symbol, Dict{Symbol, Any}}(:some => Dict(:default => 1.0, :unit => :(u"kg"), :type => Real, :description => "Some"), :more => Dict(:unit => :(u"L"), :type => Real, :description => "more"))), false)

can be instantiated like this

julia> ATest(;some=3.0u"g", name=:t)
Model t with 0 equations
Unknowns (0):
Parameters (2):
  some [defaults to 0.003]: Some
  more: more

And the units are converted correctly. Errors are given for incompatible or missing units.

Is this a desirable feature? If this PR were fleshed out to handle DynamicQuantities and array variables as well, is there a chance it could be merged?

@contradict contradict marked this pull request as draft July 26, 2024 19:12
@ChrisRackauckas
Copy link
Member

I think it's desirable. Handling DynamicQuantities is essential.

I need to evaluate the unit macro to know the types to put in the model
definition. I need a type parameter to be generic over Unitful units or
DynamicQuantities Dimensions, but that can't exist until after
evaluation.
@contradict contradict force-pushed the variable_value_units branch from 924de0f to 8c70686 Compare August 1, 2024 00:35
@contradict contradict marked this pull request as ready for review August 1, 2024 22:27
@contradict
Copy link
Contributor Author

@ChrisRackauckas I think this is ready for a review.

@ChrisRackauckas ChrisRackauckas merged commit e037dfe into SciML:master Aug 7, 2024
23 checks passed
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.

2 participants