-
Notifications
You must be signed in to change notification settings - Fork 40
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 absoluteValue inferred by default #3645
base: master
Are you sure you want to change the base?
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:
|
If \lstinline!false!, then the variable defines a relative quantity, and if \lstinline!true! an absolute quantity. | ||
If \lstinline!false!, then the component defines a relative quantity, and if \lstinline!true! an absolute quantity. | ||
When converting between units (e.g., in plots and where parameters are edited), the unit offset must be ignored for relative quantities. | ||
The annotation is inherited in the sense that when \lstinline!absoluteValue! is defined for a simple type, it also applies to components declared with the type, as well as derived classes. |
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.
Normally we use inheritance for the base-class relation (the latter part of this) not for "instantiating" a component, so couldn't we find a better word than "inherited" here?
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.
OK, here is a simple rewrite with a bit of repetition instead:
The annotation is inherited in the sense that when \lstinline!absoluteValue! is defined for a simple type, it also applies to components declared with the type, as well as derived classes. | |
The annotation is inherited in the sense that when \lstinline!absoluteValue! is defined for a simple type, it also applies derived classes. | |
When \lstinline!absoluteValue! is defined for a simple type, it also applies to components declared with the type. |
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.
The drawback of not calling it inheritance in both cases is that we then lose the ability to refer to both mechanisms in a compact manner such as (possibly through inheritance). In view of this, I would actually prefer to call it inheritance in both cases. After all, we are defining what inheritance means separately for each annotation where some sort of inheritance takes place.
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.