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 absoluteValue inferred by default #3645

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

henrikt-ma
Copy link
Collaborator

@henrikt-ma henrikt-ma commented Jan 29, 2025

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.

@HansOlsson
Copy link
Collaborator

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:

For a component where unit conversions involving non-zero offset is possible (mainly temperatures) it is recommend to give a value for absoluteValue in annotation either for the component or the type declaration. Tools may infer absoluteValue in other cases, but specifying absoluteValue reduces impact of quality-of-implementation in tool ability to infer absoluteValue.

That still gives the possibility to infer, but without making models too reliant on it.

@HansOlsson
Copy link
Collaborator

I believe the fear of over-using the annotation is misplaced. To me it is a matter of using Modelica.Units.SI.Temperature or Modelica.Units.SI.TemperatureDifference.

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).

Copy link
Collaborator

@HansOlsson HansOlsson left a 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.

@HansOlsson
Copy link
Collaborator

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:

  • Two-terms: Equality (a=b)
  • Three-terms (regardless of written as addition or subtraction a=b-c, b=a+c):
    • The difference between two absolute temperatures is a relative one.
    • Adding/subtracting two relative temperatures give a relative one.

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.

@HansOlsson HansOlsson added this to the 2025-February milestone Feb 5, 2025
@HansOlsson
Copy link
Collaborator

BTW: Such an inference has been test-implemented in Dymola, and correctly handles the sensors for absolute and relative temperature.

@henrikt-ma
Copy link
Collaborator Author

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 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 absoluteValue inference should be done.

@HansOlsson
Copy link
Collaborator

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 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 absoluteValue inference should be done.

But:

  • For units we don't tell users that inference is the default, instead we only define explicit unit-attribute in the specification. Why should we do it differently for absoluteValue based on some future work?
  • One can neither completely boot-strap unit nor absoluteValue for a model, inference starts from some variables with units and infer units for other variables.
  • For many the focus is on checking not inference; and to me that is a valid approach; by stating that inference is the default we demote that.

In practice if we look at MSL:

  • In Modelica.Blocks.Examples.Noise.Utilities.Parts.MotorWithCurrentControl (and similar ones) we can infer unit and absoluteValue for const-blocks in smpm.thermalAmbient - a nice to have, but nothing more.
  • But in Modelica.Clocked.Examples.Systems.ControlledMixingUnit I don't see how we can infer absoluteValue=true for the different temperatures in the mixing unit. The clearest indicator that it is an absolute value is exp( -weps/T), but that is really stretching it.

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.
Copy link
Collaborator

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?

Copy link
Collaborator Author

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:

Suggested change
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.

Copy link
Collaborator Author

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.

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.

Default for absoluteValue
2 participants