-
Notifications
You must be signed in to change notification settings - Fork 44
Make absoluteValue inferred by default #3645
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
Conversation
To me it doesn't make sense to specify that it is inferred without specifying how it is inferred (which to me requires unit-inferrence as we don't even know which variables require it otherwise). That's why I suggested:
That still gives the possibility to infer, but without making models too reliant on it. |
I believe the fear of over-using the annotation is misplaced. To me it is a matter of using If we consider parameters of a model I believe it is good style to declare them with a type (that includes unit and absoluteValue-annotation). Similarly for physical connectors. I would also consider it normal to declare other variables in physical models to be declared with a type (including unit and absoluteValue). The exception I see is when you build blocks interacting with such models (using sensors and sources), where I would see it as unusual to have a unit and absoluteValue (looking more I realize that there are some that have unit="K" - most, but not all implicitly have absoluteValue=true - and the exception I found was wrong in other ways as well - will open an issue). |
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.
Having inference as the default without specifying it is an issue.
It is also important that any such inference is restricted to the cases where it makes sense to avoid problems. The ones that are clear are:
The reason to be restrictive is that if tools add additional incorrect rules there might be a push to update existing good models in bad ways to circumvent those checks. Since such a scenario is not just hypothetical, I don't see how we can add inference as default without specifying the how. |
BTW: Such an inference has been test-implemented in Dymola, and correctly handles the sensors for absolute and relative temperature. |
The rules for unit checking are also important. This PR is primarily about restoring the 3.6 situation where the specification did not rule out the possibility of doing some sort of inference, but instead of just going back to the unexplained default in 3.6, this PR goes a little further by also pointing out that tools need to be aware of when their inference isn't conclusive. Similar to unit inference, we need to have faith in tools until we manage agree upon exactly how |
But:
In practice if we look at MSL:
|
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Based on discussion in #3631.
This restores the Modelica 3.6 (latest release) situation of the
absoluteValue
-annotation not having a constant default value. The difference to the 3.6 situation is that the default behavior is described in the text, as being tool inference. Similar to unit checking, the specification does not give details about how the inference shall be performed.Without directly recommending for or against explicit use of
absoluteValue
, the non-normative text is still biased by only explaining possible consequences of leaving it out, and not mentioning the drawbacks of over-specifying. This is meant to be a compromise between fear of problems when using tools that don't have good inference, and fear of pedantic/ambitious library authors putting the annotation all over their libraries in ways that might do more harm than good when using tools with good inference.Fixes #3631.