search

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
466 stars 166 forks source link

Units for mathematical constants? #4046

Open mestinso opened 1 year ago

mestinso commented 1 year ago

...reading through some of the recent activity in the Modelica spec(https://github.com/modelica/ModelicaSpecification/issues/3259 and https://github.com/modelica/ModelicaSpecification/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/modelica/ModelicaStandardLibrary/issues/3158

mestinso commented 1 year ago

@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 commented 1 year ago

@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:

henrikt-ma commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

HansOlsson commented 1 year ago

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 commented 1 year ago

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

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