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

Units for mathematical constants? #4046

Open
mestinso opened this issue Oct 7, 2022 · 15 comments · Fixed by #4155
Open

Units for mathematical constants? #4046

mestinso opened this issue Oct 7, 2022 · 15 comments · Fixed by #4155
Labels
L: Constants Issue addresses Modelica.Constants
Milestone

Comments

@mestinso
Copy link
Collaborator

mestinso commented Oct 7, 2022

...reading through some of the recent activity in the Modelica spec(modelica/ModelicaSpecification#3259 and modelica/ModelicaSpecification#2127 and others), it seems like it might be a natural step to specify the units for the various mathematical constants in Modelica.Constants.

I might propose the following, which seems a bit more correct:

final constant Real e(unit="1")=Modelica.Math.exp(1.0);
final constant Real pi(unit="1")=2*Modelica.Math.asin(1.0); // 3.14159265358979;
final constant Real D2R(unit="rad/deg")=pi/180 "Degree to Radian";
final constant Real R2D(unit="deg/rad")=180/pi "Radian to Degree";
final constant Real gamma(unit="1")=0.57721566490153286061

Without this step, it seems to me like we are missing out on some unit checking opportunities since the units for pi (and others) can be inferred incorrectly.

Thoughts/comments?

@henrikt-ma
Copy link
Contributor

I wouldn't worry about the ones with unit = "1" until we know what is obtained automatically by a refined language specification. (Personally, I'd prefer a language where it is perfectly fine to leave out the explicit unit = "1" as a way of defining a dimensionless variable, but time will tell…)

Regarding the others, what is the expected status of "deg" in Modelica? Consider:

Real a(unit = "1") = 1.0;
Real b(unit = "rad") = 1.0;
Real c(unit = "deg") = 1.0;
Real y = sin(a) + sin(b) + sin(c); /* OK? */

Are tools expected to know that sin operates both on unit "1" and "rad", but not on "deg"?

@HansOlsson
Copy link
Contributor

Are tools expected to know that sin operates both on unit "1" and "rad", but not on "deg"?

The original idea was that variables shall be in base-SI-units, so variables shall not have unit="deg".

@henrikt-ma
Copy link
Contributor

henrikt-ma commented Oct 10, 2022

Right, but then someone came up with Angle_deg and used it in Modelica.Mechanics.MultiBody.Parts.FixedRotation, so "deg" has a pretty widespread use by now…

@HansOlsson
Copy link
Contributor

Right, but then someone came up with Angle_deg and used it in Modelica.Mechanics.MultiBody.Parts.FixedRotation, so "deg" has a pretty widespread use by now…

And, of course, there was originally a bug in using them. The better solution is to correct that at some point in the future; as indicated by #3158

@mestinso
Copy link
Collaborator Author

@henrikt-ma For pi and other unit="1" items: if you think it's prudent to wait then that seems reasonable. I will just say I hope/expect I should be able to catch the following unit errors in the near future:

Real T(unit="K") = 300 "Temperature";
Real r(unit="m") = 1 "Radius";
Real A(unit="m2") = Modelica.Constants.pi*r^2*T "Area"; // T shouldn't be here
Real C(unit="m") = 2*Modelica.Constants.pi*r*T "Circumference"; // T shouldn't be here

As is, at least in the Modelica tool I'm using today, neither of those unit issues would be caught. The first seems to largest offender in my mind because conceptually there is no good reason it's not caught. The proposed change would address that today without waiting for changes to spec and waiting for agreement on how to handle situations when unit="". The 2nd seems potentially a little trickier because now we have two terms (2, pi) that each doesn't have units. The fact that adding unit="1" addresses at least the first case today and potentially simplifies the assumptions in the 2nd case (only need to assume units for 2 and not both 2 and pi), is the main reason and thinking that prompted me to create this issue ticket.

Regarding the R2D and D2R items, I have a few more comments:

  1. These don't appear to be in use in MSL. Probably they should show up in the to_deg and from_deg conversions at least. Otherwise, why even keep them if they aren't used in the most natural spot (conversion function)?
  2. Even if they are kept, they are redundant (just reciprocals), do we need both of them? We don't have any other reciprocal constants.
  3. Since nonSI deg units are in use in MSL and even this file (considering T_zero with degC), it does seem ok and correct from my standpoint to tack on the units.

@henrikt-ma
Copy link
Contributor

henrikt-ma commented Oct 13, 2022

@henrikt-ma For pi and other unit="1" items: if you think it's prudent to wait then that seems reasonable. I will just say I hope/expect I should be able to catch the following unit errors in the near future:

I cannot imagine that we'd consider MCP-0027 resolved without being able to reject both of these with support in the specification. For example, this is how 2 * Modelica.Constants.pi * r * T would be rejected according to #3257:

  • Left associativity means that the expression is parsed as (((2 * Modelica.Constants.pi) * r) * T) (but the end result should be the same also if the factors are reordered).
  • 2 is implicitly cast to Real in order for the multiplication with the Real Modelica.Constants.pi to be defined. Hence 2 has empty unit.
  • Modelica.Constants.pi is a (constant) component reference outside of a function, where the declared unit is "". Hence, the expression Modelica.Constants.pi has unit "1".
  • Since one of the operands in 2 * Modelica.Constants.pi has non-empty unit (namely Modelica.Constants.pi with unit ""), the inferred unit of 2 defaults to "1" in order obtain the unit for the product.
  • The unit of 2 * Modelica.Constants.pi is the product "1" * "1" = "1".
  • The unit of 2 * Modelica.Constants.pi * r * T is the product ("1" * "m") * "K" = "m.K".
  • This is a violation of dimensional homogenity for a declaration equation.

@henrikt-ma
Copy link
Contributor

Regarding the R2D and D2R items, I have a few more comments:

  1. These don't appear to be in use in MSL. Probably they should show up in the to_deg and from_deg conversions at least. Otherwise, why even keep them if they aren't used in the most natural spot (conversion function)?
  2. Even if they are kept, they are redundant (just reciprocals), do we need both of them? We don't have any other reciprocal constants.

Perhaps only to not introduce a backwards incompatible change in the MSL?

Note that it could be tricky do introduce a library conversion annotation that would describe what to do as a replacement for referencing these constants, as it's currently not on the table to perform unit conversions using a Modelica expression. For example, we are currently not considering anything like unit(1, "rad/deg"), which would allow the 1 with empty unit (due to being implicitly cast to Real) to get inferred unit "rad/deg".

@henrikt-ma
Copy link
Contributor

Regarding the mention of #2127 above, the unit checking discussions are now going in the direction of having special semantics for constants, deprecating declaration without explicitly setting unit. The idea is that we should have:

  final constant Real inf(unit = "") = ModelicaServices.Machine.inf "Biggest Real number such that inf and -inf are representable on the machine";

as the only way of saying that inf will act similarly to a Real literal when used in expressions, for example:

    parameter SI.Time startTime = -Modelica.Constants.inf "Output = false for time < startTime";

This would be totally in agreement in the original suggestions made in this issue. I have a hard time seeing that we would regret introducing these explicit units already today, even if the unit checking discussions in MCP-0027 are far from settled.

@henrikt-ma
Copy link
Contributor

henrikt-ma commented Jun 13, 2023

For completeness and to support the unit checking work, we could also consider adding explicit unit = "" for the remaning constants without unit, and then also add it to corresponding constants in ModelicaServices.Machine. However, I would prefer having separate pull requests for non-empty and empty unit modifications, as the deprecation of omitting the empty ones is a more open unit checking design problem.

@HansOlsson
Copy link
Contributor

Regarding the mention of #2127 above, the unit checking discussions are now going in the direction of having special semantics for constants, deprecating declaration without explicitly setting unit. The idea is that we should have:

  final constant Real inf(unit = "") = ModelicaServices.Machine.inf "Biggest Real number such that inf and -inf are representable on the machine";

as the only way of saying that inf will act similarly to a Real literal when used in expressions, for example:

    parameter SI.Time startTime = -Modelica.Constants.inf "Output = false for time < startTime";

This would be totally in agreement in the original suggestions made in this issue. I have a hard time seeing that we would regret introducing these explicit units already today, even if the unit checking discussions in MCP-0027 are far from settled.

We can also separate them into the ones with explicit unit="1" like pi. I don't see any harm in that at all.

Whether we should add unit="" for the other ones seems less clear.

One possibility is that we introduce a rule that an explicit unit is required, and in that case it sort of circumvents that check, another is that there are no special rules for constants (in which case it would be redundant; but sort of ok), and a third is that the special rule is that we for constants (or possibly all variables with a definition equation) the unit can only be derived from its definition equation (in which case it is redundant; but sort of ok).

@henrikt-ma
Copy link
Contributor

We can also separate them into the ones with explicit unit="1" like pi. I don't see any harm in that at all.

Sure, I'd welcome a PR on that to start with.

Whether we should add unit="" for the other ones seems less clear.

One possibility is that we introduce a rule that an explicit unit is required, and in that case it sort of circumvents that check, another is that there are no special rules for constants (in which case it would be redundant; but sort of ok), and a third is that the special rule is that we for constants (or possibly all variables with a definition equation) the unit can only be derived from its definition equation (in which case it is redundant; but sort of ok).

How would you fix the following if the unit can only be derived from the definition equation?

  final constant Real G(final unit = "m3/(kg.s2)") = 6.67408e-11 "Newtonian constant of gravitation (previous value: 6.6742e-11)";

@HansOlsson
Copy link
Contributor

We can also separate them into the ones with explicit unit="1" like pi. I don't see any harm in that at all.

Sure, I'd welcome a PR on that to start with.

Good that we have agreement.

How would you fix the following if the unit can only be derived from the definition equation?

  final constant Real G(final unit = "m3/(kg.s2)") = 6.67408e-11 "Newtonian constant of gravitation (previous value: 6.6742e-11)";

I meant that it can be given a unit or derived from the definition equation, but not inferred in other ways.

@henrikt-ma
Copy link
Contributor

How would you fix the following if the unit can only be derived from the definition equation?

  final constant Real G(final unit = "m3/(kg.s2)") = 6.67408e-11 "Newtonian constant of gravitation (previous value: 6.6742e-11)";

I meant that it can be given a unit or derived from the definition equation, but not inferred in other ways.

That makes more sense. However, I would only want this when the definition equation comes with a concrete unit, so that constant Real pi = 3.14 still wouldn't be allowed.

@beutlich beutlich added the L: Constants Issue addresses Modelica.Constants label Aug 9, 2023
@beutlich beutlich added this to the MSL4.1.0 milestone Aug 9, 2023
@HansOlsson
Copy link
Contributor

I think we might revisit this for pi after looking at Modelica.Electrical.Analog.Examples - with examples such as CompareTransformers, SeriesResonance where inductance equations involve pi in a way that seems unit-odd (clearly there other unit issues with CompareTransformers). If user models has similar models this change may cause the impression that this is not a minor upgrade for users.

I don't see a similar issue with the other constants.

@HansOlsson HansOlsson reopened this Sep 15, 2023
@HansOlsson
Copy link
Contributor

HansOlsson commented Sep 15, 2023

To clarify how significant: I enabled the proposed unit-checking in Dymola and tested the master-branch, and there were:

  • 2301 warnings on master branch.
  • 1224 warnings on master if I removed pi.unit="1"
    (Most (or almost all) are unit-related.)

Clearly it is in one sense good to make the change, but I don't think we can focus that much on those issues.

HansOlsson added a commit to HansOlsson/Modelica that referenced this issue Sep 18, 2023
 since it is causing too many regressions for a minor release.

With proposed unit-checking in Dymola this reduces the warnings from 2301 to 1224 (Most - but not all are unit-related.)

Obviously many of them could be corrected, but I don't see that we have the resources to focus on that for this minor release.
The first models found were CompareTransformers, SeriesResonance in Modelica.Electrical.Analog.Examples.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Constants Issue addresses Modelica.Constants
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants