modelica / ModelicaSpecification

Specification of the Modelica Language
https://specification.modelica.org
Creative Commons Attribution Share Alike 4.0 International
98 stars 42 forks source link

MCP-0027 Units of Literal Constants #2127

Open modelica-trac-importer opened 5 years ago

modelica-trac-importer commented 5 years ago

Reported by fcasella on 11 Dec 2016 19:03 UTC I have added a proposal for the clarification of the units of literal constants, which is currently unspecified in Modelica 3.3r1. I hope there's still time to have it in Modelica 3.4, the proposal is very simple and the evaluation process should be straightforward.

Documents can be accessed directly here: Overview and Specification Changes.


Migrated-From: https://trac.modelica.org/Modelica/ticket/2127

henrikt-ma commented 2 years ago

I'd like to simplify the proposal further, and I hope that this would make it even easier to reach agreement.

Only allow unit inference for just-a-literal declaration equations. This means that unitful literals are only allowed in places that allow them to be seen and manipulated with a choice of displayUnit. Consequences of this include:

henrikt-ma commented 2 years ago

The output of transcendental built-in functions such as sin(), cos(), exp(), etc. is implicitly assumed to have unit "1" if they are called within expressions containing variables, parameters, or constants that have a non-empty unit attribute string.

What is the intention behind this? When should the output of these functions be given any other unit than "1"?

bilderbuchi commented 2 years ago

The output of transcendental built-in functions such as sin(), cos(), exp(), etc. is implicitly assumed to have unit "1" if they are called within expressions containing variables, parameters, or constants that have a non-empty unit attribute string.

What is the intention behind this? When should the output of these functions be given any other unit than "1"?

I'm assuming it is to keep the scope of applicability identical between output-of-transcendental-function and literal-Real-and-Integer-constants, for easier reasoning and to limit the scope of this change to the minimum necessary (the "baby-steps" approach).

bilderbuchi commented 2 years ago

I'd like to simplify the proposal further, and I hope that this would make it even easier to reach agreement.

Only allow unit inference for just-a-literal declaration equations. This means that unitful literals are only allowed in places that allow them to be seen and manipulated with a choice of displayUnit. Consequences of this include:

* We would be rejecting `specificEnthalpy_pT(1e5, 293.15)`, meaning that it would need to be rewritten using suitably defined constants, which would seem like a nice idea anyway.

* We would be rejecting `SI.Length L = 10.0*2`, reflecting the fact that the `2` in here is probably meant to be a dimensionless number.  The workaround, of course, is to make `10.0` its own declaration equation where the unit can be inferred.

I'm not sure it would be easier to reach agreement on this alternative, given that there were some concerns raised and disagreement earlier about the proposal to reject specificEnthalpy_pT(1e5, 293.15) usage.

NB: I agree that rejecting this would be "nicer"/more "proper"! At the same time, the concern raised about user acceptance seems to me quite valid.

At this point in the discussion, I agree with @casella that we should salvage the small, useful, and noninvasive change that he proposes to avoid the "wildcard effect", and move further refinements/scope extensions/discussions to new issues. :+1:

bilderbuchi commented 2 years ago

@casella, a detail that was not fully clear to me:

The output of transcendental built-in functions such as sin(), cos(), exp(), etc. is implicitly assumed to have unit "1" if they are called within expressions containing variables, parameters, or constants that have a non-empty unit attribute string.

Your proposal text considers the output of built-in transcendental functions, but what about the input of built-in transcendental functions? Building on the already present example

  • i = i_0*exp(v/v0) is accepted

would i = i_0*exp(3*v) be rejected or accepted/considered out of scope of the current proposal? (I assume the latter)

henrikt-ma commented 2 years ago

I'm not sure it would be easier to reach agreement on this alternative, given that there were some concerns raised and disagreement earlier about the proposal to reject specificEnthalpy_pT(1e5, 293.15) usage.

While I'm not a fan of the idea, the most harmless way of allowing h_pT(1e5, 293.15) could be to allow unit inference for just-a-literal function call argument expressions. This way, it is easily seen exactly where the inferred unit gets applied, and the main drawback that I can see is then just that the lack of displayUnit at the place of the literal (the fact that the function can declare a displayUnit is of no help at the call site) makes it less convenient to work with the expression in a unit-equipped expression editor.

casella commented 2 years ago

I'd like to simplify the proposal further, and I hope that this would make it even easier to reach agreement.

Only allow unit inference for just-a-literal declaration equations. This means that unitful literals are only allowed in places that allow them to be seen and manipulated with a choice of displayUnit.

@henrikt-ma I'm not sure I understand exactly what your proposal is. Could you please write it down in the form of an amendment to the specification text?

Also, why should we mention "unit inference" at all? The scope of this ticket is to determine when literal units should be given a "1" unit, so that unit checking (not inference) is indeed possible in expressions containing them. Do I miss something?

* We would be rejecting `specificEnthalpy_pT(1e5, 293.15)`, meaning that it would need to be rewritten using suitably defined constants, which would seem like a nice idea anyway.

* We would be rejecting `SI.Length L = 10.0*2`, reflecting the fact that the `2` in here is probably meant to be a dimensionless number.  The workaround, of course, is to make `10.0` its own declaration equation where the unit can be inferred.

We may start another discussion here about that, but the fact is, this patterns are indeed used. To be honest, I'm not against using Modelica as a calculator, as long as only literal constants is used. Nobody gets harmed by that, why make it illegal?

The scope of this proposal is as minimalistic as possible, the only cases that would no longer pass dimensional checking are the one where literals and variables/parameters are mixed. I'm afraid this is really necessary if we want to solve the original issue, i.e. the wildcard effect of literal constants.

casella commented 2 years ago

@bilderbuchi I agree with you last comment, I edited my proposal accordingly

henrikt-ma commented 2 years ago

I'm reordering questions a bit in the answer below.

We may start another discussion here about that, but the fact is, this patterns are indeed used. To be honest, I'm not against using Modelica as a calculator, as long as only literal constants is used. Nobody gets harmed by that, why make it illegal?

For two different – and somewhat weak – reasons:

The last of these points is a nice feature I'd be ready to give up in return for the full power of something based on a bottom-up analysis. Fortunately, this problem is specific to fancy application of displayUnit, and isn't a concern for unit checking.

I'd like to simplify the proposal further, and I hope that this would make it even easier to reach agreement. Only allow unit inference for just-a-literal declaration equations. This means that unitful literals are only allowed in places that allow them to be seen and manipulated with a choice of displayUnit.

@henrikt-ma I'm not sure I understand exactly what your proposal is. Could you please write it down in the form of an amendment to the specification text?

Actually, I'd much rather go back to a solution based on the first steps towards a complete unit system based on bottom-up analysis. I'll try to repeat this with a concise summary in a separate comment below.

Also, why should we mention "unit inference" at all? The scope of this ticket is to determine when literal units should be given a "1" unit, so that unit checking (not inference) is indeed possible in expressions containing them. Do I miss something?

Probably that "unit inference" is what I mean is going on here:

Real l(unit = "m") = 5; /* Inferred unit of "5" is "m". */
henrikt-ma commented 2 years ago

This is a summary of ideas presented almost four years ago, found in earlier comments of this issue. This time, I'll not talk about how the idea can be refined in the future, as such talk seems to cause more confusion than building confidence in the basic ideas.

Design

Concepts

Basic concepts:

I say may treat as error, as a large gray zone is a feature that has been requested in the comments above.

Unit inference

The empty unit can always be implicitly cast to unit "1". In some cases the empty unit can be implicitly cast to an inferred unit:

(The last rule above has been added now due to popular demand. To keep the design close to the bare minimum, the more complete proposal above's set of rules for translation-time constants with value 0.0 is excluded.)

Expressions with empty unit

What remains is to design precisely which expressions that have empty unit, and it needs to be understood that the rules proposed here is just a starting point for something that should grow into a more complete solution in the future.

Basic expressions defined to have empty unit:

Expressions defined to not propagate the empty unit, thereby forcing inference of unit "1":

Built-in non-array operators, functions, and special expressions that propagate any unit (including empty):

Special situations in which the empty unit can be propagated up the expression tree:

Examples

For a fair comparison with the alternative design above, assume we have rules like these that are unrelated to the problem with units of literal constants:

Same list of examples as in https://github.com/modelica/ModelicaSpecification/issues/2127#issuecomment-1184636464:

Additional examples:

Some of the examples above are included just to point out how the design could be extended in the future.

bilderbuchi commented 1 year ago

@henrikt-ma could you maybe cast your proposal as a concrete amendment to the specification text, as Francesco requested above? (Plus the set of illustrative examples) I think this will make it easier to evaluate and compare the two proposals side-by-side and to weigh the respective merits (e.g., simplicity, conciseness, future extensibility,...) of the two proposals in solving the stated problem/topic of this issue, i.e. "Units of Literal Constants". 😃 Thanks!

casella commented 1 year ago

Probably that "unit inference" is what I mean is going on here:

Real l(unit = "m") = 5; /* Inferred unit of "5" is "m". */

@henrikt-ma, thanks for pointing this out! This is exactly where your approach and my proposal fundamentally differ.

What I am proposing is to only infer that the unit of literals is "1" in some specific cases, so we can successfully determine that v = sqrt(2*g) is dimensionally inconsistent. That's it.

I'm not trying to attempt to infer the unit of all literals constants, in particular the ones that are supposed to have one which is not "1" (pun intended). That's a way more complex problem. Which I believe is not necessary to solve in order to address the issue of this ticket, i.e., avoiding the "wildcard effect" of literals in expressions with variables that do have a unit. This is an actual end-user requirement, not just a fancy CS theoretical issue involving type systems. If I and my co-workers had this simple feature implemented while doing modelling work, it would have saved us a lot of time and pain.

I think the main appeal of my proposal it is that it is very simple (just adding four lines of text to the Modelica spec), it is easy to implement, it solves the problem of missing many obviously dimensionally inconsistent (= plain wrong) equations that current tools do not catch, and it does not introduce any backwards incompatibility whatsoever on commonly used Modelica coding patterns.

From this point of view, the right title for my MCP proposal should be "Dimensionless literal constants", rather than "Units of Literal Constants", which is much broader. This explains exactly the scope I wanted to cover since I raised this issue several years ago.

As I understand, your proposal aims to be more general, in the sense that it would obtain the results that my proposal would obtain, plus many others, at the expense of being more elaborate, more involved to get an agreement on, possibly more difficult to implement, and of introducing some (mild) backwards incompatibility.

I am sorry to repeat myself, but I really see no point for controversy here. These are not two mutually exclusive proposals. One is less general, apparently with no controversial issues except that it doesn't solve other problems beyond the literal wildcard unit effect, while the other is more general, adds some extra features (for which there is no pressing request from end-users, AFAIK) but has some drawbacks.

My recommendation after so many years of endless discussion and zero results on this topic is to approve the less general and not controversial proposal about "Dimensionless Literal Constants". Unless of course it has other faults beyond not solving all problems; so far I haven't heard about any.

Then we can continue the discussion on the more general but still controversial proposal of yours 😅. If that eventually gets into the specification, it will replace my four lines with something broader in scope. In the meantime, actual end-users developing new equation-based Modelica models will benefit from improved sanity checks in Modelica tools. Which is my primary goal here.

Do we have some agreement on that?

HansOlsson commented 1 year ago

I'm not trying to attempt to infer the unit of all literals constants, in particular the ones that are supposed to have one which is not "1" (pun intended). That's a way more complex problem. Which I believe is not necessary to solve in order to address the issue of this ticket, i.e., avoiding the "wildcard effect" of literals in expressions with variables that do have a unit. This is an actual end-user requirement, not just a fancy CS theoretical issue involving type systems. If I and my co-workers had this simple feature implemented while doing modelling work, it would have saved us a lot of time and pain.

I can understand this, and I also believe that we have gotten serious mission creep in this proposal.

While this PR has been discussed I have made the following PRs for MSL: https://github.com/modelica/ModelicaStandardLibrary/pull/2375 https://github.com/modelica/ModelicaStandardLibrary/issues/2377 (well, issue but still) https://github.com/modelica/ModelicaStandardLibrary/pull/2378 https://github.com/modelica/ModelicaStandardLibrary/pull/3881 https://github.com/modelica/ModelicaStandardLibrary/pull/4040 https://github.com/modelica/ModelicaStandardLibrary/pull/4041

This indicates that actually having some non-perfect unit-checking in Modelica is important - and that introducing something corresponding to those four lines actually can make life better for users.

henrikt-ma commented 1 year ago

@henrikt-ma could you maybe cast your proposal as a concrete amendment to the specification text, as Francesco requested above? (Plus the set of illustrative examples) I think this will make it easier to evaluate and compare the two proposals side-by-side and to weigh the respective merits (e.g., simplicity, conciseness, future extensibility,...) of the two proposals in solving the stated problem/topic of this issue, i.e. "Units of Literal Constants". 😃 Thanks!

Sure. I'm setting up this MCP according to the standard form, so that I can make my proposal in the form of an PR to the main branch of this MCP:

Now that the structure has been set up, the "four-line" alternative could also be presented in the form of another PR towards the main MCP branch. This way, we can have separate discussions about the pros and cons of each alternative.

casella commented 1 year ago

Thanks @henrikt-ma for setting this up, I will follow suit with a sub-PR for my proposal.

Before getting into the actual MCP text of either proposal, I would make sure we are all on the same page with respect to some basic concepts which apply to both proposals. Hence, I'm putting the discussion here. I think it is important that we all agree on them before carving text in stone with an MCP, that we may later regret about.

I apologize for coming up late with this discussion, but the whole thing became clear to me only very recently.

Current status with Modelica 3.5

AFAIK, there is no other place in the specification saying anything about the semantics of the unit and displayUnit attributes, nor any further indication about their purpose and about how they could actually be used.

It is commonly understood in the community that units can be used for "unit checking" and "unit inference" or "unit deduction" (see, e.g., this paper). Units and displayUnits can obviously be employed for the purposes of unit conversion in parameter input dialogs and for display purposes in plots. Also, since Chapter 19 mandates unit expressions to be SI units, it is also implicitly assumed that parameter input and simulation result outputs in non-SI units should be handled by means of displayUnit only.

However, none of these things is standardized, nor even hinted at in non-normative parts of the Specification.

This leaves a lot of room for tool-specific interpretation, which is perhaps not ideal for people who'd like to write portable, tool-independent code; on the other hand, what is actually stated in the current specification about units (very little) is not controversial at all.

Unit checking vs Dimensional consistency checking

Consider the following fragment of Modelica code (not a valid model, but that is irrelevant for this discussion).

  Real theta(unit="rad") "wheel rotation angle";
  Real w(unit="rad/s") "wheel angular speed";
  Real f(unit="Hz") "number of rotations per unit time";
  Real L(unit="m") "travelled distance";
  parameter Real R(unit="m") "wheel radius";
  parameter Real V(unit="m3") "volumetric pump displacement";
  parameter Real q(unit="m3/s") "volume flow";
equation
  der(theta) = w;
  der(theta) = f;
  L = theta*R;
  q = f*V;
  q = omega*V;

The five equations shown above are all dimensionally consistent: the first two have time^-1 on both sides, the third has length on both sides, the last two have length^3/time on both sides. If we perform dimensional consistency checking, all five are thus expected to pass. Note that rad is a dimensionless unit (see this excellent paper for a thorough discussion of this concept).

From basic physics, we know that the second and the last equations are wrong, as there is a 2*pi conversion factor missing somewhere. Can we tell that by unit checking? The answer is unfortunately no:

Bottom line: there is no way an algorithm can tell anything definite about the correctness of equations by looking at their SI units. Units could be right and equation wrong, or viceversa. You need expert human understanding about what those equations actually mean to figure that out.

It is definitely possible (and indeed very useful to catch gross modelling errors) to perform dimensional checking on expressions and equations using variables with unit attributes, but that is limited to checking the consistency of the dimensions of their terms, not of their actual SI units. From this point of view, "rad" and "1" are equivalent, and so are "Hz", "1/s", and "rad/s", even though some of those units, e.g. "Hz" and "rad/s" are definitely different.

Unit inference vs Dimensional inference

The arguments of the previous section can now be used in reverse for the topic of unit inference, which is involved in @henrikt-ma's proposal. Consider the following additional fragment to the Modelica code presented above

  Real theta1 "Rotational angle, no unit given";
  Real omega1 "Angular speed, no unit given";
  Real f1 "Rotations per unit time, no unit given";
equation
  L = omega1*R;
  q = omega1*V;
  q = f1*V;

Any "unit inference" algorithm (no matter how cleverly designed) when applied to the first equation will always conclude that omega1 has unit "1", so as to get "m" on both sides. There's no way it can figure out the correct unit "rad".

Ditto for the second equation, where the unit of omega1 will be inferred to be "1". No way "rad" can be inferred. By the way, if omega1 is determined from the first equation, it is really an angular velocity, so plugging it in the second equation, that requires a rotational frequency instead, will always lead to a wrong result by a factor 2*pi. Un-catched wrong equation and wrong inferred unit.

Regarding the third equation, f1 will get "1/s", though "Hz" will probably be a better choice.

Bottom line: in general we can only perform "dimensional inference", but most definitely not "unit inference", as the above examples clearly demonstrate.

Do we have a consensus on everything I stated in this comment? Only then I believe we can start actually delving into the concrete MCP text.

mestinso commented 1 year ago

@casella Unless the more radical approach of treating rad as a full-fledged base unit was taken, then I fully agree with your comments and conclusions in your last post. With that said, if rad were taken as a base unit, then only your 1st and 4th equations would pass unit checks and the others would be flagged as not passing and the user would be expected to add in unit rad factor in case of equation 3 and 2*pi rad factors in the case of equations 2 and 5. However, given the current state in the SI system of rad not being a base unit and being defined as equal to 1, then I don't think such an approach should practically be taken since a) it's not the norm and b) so many equations would need to be adjusted to add in unit rad factors.

With that said, I think a sort of hybrid approach could be taken (recognizing that there is a strong argument for rad being a base unit, but short of enforcing it).

For example, I would argue in the following case that w should have units inferred to be "rad/s" rather than "1/s"

  Real theta(unit="rad") "wheel rotation angle";
  Real w "wheel angular speed";
equation
  der(theta) = w;

Basically, I would say from a pure unit checking standpoint, treat rad==1, but from a unit inference standpoint, treat rad as a base unit.

sjoelund commented 1 year ago

If someone has time to check: what would happen to user libraries if rad was added as a base unit to the unit checker?

Even if it made something more complicated to describe in Modelica, you could avoid a lot of problems if angles were checked better.

Inferring w=rad/s vs 1/s doesn't change anything in success/warning of the unit checker because they are the same. Passing a wrong unit=1 value to a trig function is very easy to do.

(But it's also orthogonal to this ticket)

mestinso commented 1 year ago

One other very recent relevant paper here (same author as the one @casella previously linked) that may be of interest is here: https://iopscience.iop.org/article/10.1088/1681-7575/ac7bc2

The author argues for the approach of elevating the angle as its own dimension and rad as the base unit. Also, I suppose Hz would be downgraded as not being SI coherent (presumably we would no longer see the 2 pi conversion factors in base SI equation sets).

Note that these authors are part of an SI working group working on Angles among other things: https://www.bipm.org/en/committees/cc/ccu/wg/ccu-wgadq/members

HansOlsson commented 1 year ago

If someone has time to check: what would happen to user libraries if rad was added as a base unit to the unit checker?

That would require a quite major change in Modelica.Mechanics - in particular for Rotational.

We currently have type MomentOfInertia Real(unit="kg.m2") and type Torque=Real(unit="N.m"), and for a rotational inertia energy=J*w^2 and J*a=tau. To me the unit for MomentOfInertia seems quite natural, so I don't see how they plan to handle that.

Added: Looking at the reference rotations - it seems that rotations are handled, but not in a nice way. Read 5.1 in the reference where they define a_c=r*(2*pi*omega/Theta)^2, and for radians Theta=2*pi*rad, and they use a_c=r*omega_rad^2 where omega_rad=omega/(1 rad). That's someone blinded by their own formalism instead of re-evaluating it.

henrikt-ma commented 1 year ago

Bottom line: in general we can only perform "dimensional inference", but most definitely not "unit inference", as the above examples clearly demonstrate.

Do we have a consensus on everything I stated in this comment? Only then I believe we can start actually delving into the concrete MCP text.

I think you may have read too much into the presence of unit inference in #3257. In that proposal, unit inference is only allowed where there's one and only one obvious unit candidate for the inference. Hence, I don't see that any of the concerns you raise regarding unit inference would apply to my proposal.

Further, #3257 doesn't involve any "dimensional inference"; the very limited unit inference is considered sufficient.

henrikt-ma commented 1 year ago

If someone has time to check: what would happen to user libraries if rad was added as a base unit to the unit checker?

That would require a quite major change in Modelica.Mechanics - in particular for Rotational.

We currently have type MomentOfInertia Real(unit="kg.m2") and type Torque=Real(unit="N.m"), and for a rotational inertia energy=J*w^2 and J*a=tau. To me the unit for MomentOfInertia seems quite natural, so I don't see how they plan to handle that.

When this happens, isn't the solution to modify some of the equations by multiplication of a 1 with unit "rad" raised to a suitable power? Except that it involves a bit of work, I think the main drawback of such an approach is that formulas might end up not looking exactly like in the textbooks.

mestinso commented 1 year ago

We currently have type MomentOfInertia Real(unit="kg.m2") and type Torque=Real(unit="N.m"), and for a rotational inertia energy=J*w^2 and J*a=tau. To me the unit for MomentOfInertia seems quite natural, so I don't see how they plan to handle that.

Another paper and some relevant tables (same author as the previous material on angles): https://arxiv.org/pdf/1409.2794.pdf

Screen Shot 2022-10-10 at 12 55 52 PM Screen Shot 2022-10-10 at 11 42 48 AM

...maybe an issue with the centrifugal force?

I will say it feels more natural having the denominator of rad for torque. It is a little bothersome to me that torque has units of work. Rather in this framework it's the more intuitive work per unit rotation (or angle). In the same sense that for a linear force it's work per unit distance.

Sort of an interesting topic, but not clear if the juice is worth the squeeze here.

HansOlsson commented 1 year ago

When this happens, isn't the solution to modify some of the equations by multiplication of a 1 with unit "rad" raised to a suitable power?

The problem is that it is basically cheating - as soon as you allow formulas to have unitRad in them you have the lost the benefit of unit-checking - and you get things like "centrifugal force" having unit="N.rad" as if it were different from other forces.

mestinso commented 1 year ago

When this happens, isn't the solution to modify some of the equations by multiplication of a 1 with unit "rad" raised to a suitable power?

The problem is that it is basically cheating - as soon as you allow formulas to have unitRad in them you have the lost the benefit of unit-checking - and you get things like "centrifugal force" having unit="N.rad" as if it were different from other forces.

A minor note on the centrifugal force result as pictured in the table is that the 2022 paper actually specifically calls out and corrects that error. The table is primarily derived from a rule of thumb, but that rule of thumb apparently doesn't apply to that particular case.

Another comment I would give is that the proposed/implied changes from the discussed literature seem to have two different flavors: 1) In some cases, the equations are fully unchanged, but the units for the terms are "upgraded" to include radians. Examples include tau=I*alpha, E=0.5*I*omega^2, L=I*omega, and omega=d/dt theta (and others). For these cases, the unit checking comes automatically. 2) In other cases, the rad factors need to be added. Examples include sin(theta/(1 rad)), exp(i*theta/(1 rad)), or a_c=(omega/(1 rad))^2*R, omega=sqrt(g/L) * 1 rad, and s=(theta/(1 rad))*r. These are the ones that feel a little like "cheating", but on the other hand, they do appear logically necessary for things like sin and exp which should have dimensionless inputs. And, related to one of @casella 's points, without these explicit factors, it is otherwise impossible to deduce units in many situations, so in a way, it should be no surprise that explicit addition of units is necessary sometimes.

HansOlsson commented 1 year ago

To me treating "rad" as sort of "1" avoids adding those odd factors, and still helps. You will be unable to "deduce units" for unit-conversion - but Modelica doesn't have automatic unit-conversions in models.

More importantly I don't see that it will necessarily reduce the number of problems so significantly that it is worth the effort - as the rad-factors and similar unit-factors can also be applied incorrectly; as seen in https://github.com/modelica/ModelicaStandardLibrary/pull/4040

At least this can wait until we have the other unit-checking things in place.

HansOlsson commented 1 year ago

Going back to Francesco's proposal I see that it importantly states expressions only containing numbers are fine, and not only expressions that consists of just one number.

That's important and Dymola's (not officially released (*) ) current unrelaxed unit-checking doesn't allow that (it opts for the second alternative) and thus e.g., Modelica.Electrical.Analog.Examples.CauerLowPassAnalog causes problems due to:

  parameter SI.Capacitance c2=1/(1.704992^2*l1)
    "Filter coefficient c2";
  parameter SI.Capacitance c3=1.682 "Filter coefficient c3";
  parameter SI.Capacitance c4=1/(1.179945^2*l2)
    "Filter coefficient c4";

I'm not saying that it is the best way of writing such parameter bindings, but I can understand that we allow it - while still detecting the problem in2*c4-c2*c3

*: Obviously, we plan on improving it before releasing it.

henrikt-ma commented 1 year ago

Going back to Francesco's proposal I see that it importantly states expressions only containing numbers are fine, and not only expressions that consists of just one number.

That's important and Dymola's (not officially released (*) ) current unrelaxed unit-checking doesn't allow that (it opts for the second alternative)

It was also my impression some time ago that such strict rules were desired, as it would allow every inferred unit to be attached to a particular literal in a formula. However, the discussion in #2127 made it clear to me that it was generally considered too restrictive, and #3257 has been designed with the more relaxed approach in mind.

I'm not sure what is the desired result here, to me the c2 and c4 look like the kind of problems we'd like to detect. This is how #3257 handles the declaration equation c2 = 1 / (1.704992^2 * l1), from the bottom of the expression tree and up, as usual:

Edit: I suppose a unit-correct way of expressing the declaration equation would require the use of an auxiliary constant to which the missing unit can be attached, for example:

protected
 constant Real t2(unit = "s") = 1 / 1.704992;
public
  parameter SI.Capacitance c2 = t2^2 / l1
    "Filter coefficient c2";
HansOlsson commented 1 year ago

I'm not sure what is the desired result here, to me the c2 and c4 look like the kind of problems we'd like to detect.

Ah, sorry - I was tired and saw it as 11 not l1; so probably Dymola did the right thing.

casella commented 1 year ago

See PR #3266 with my concrete proposal for amending the Specification text.

bilderbuchi commented 1 year ago

From the various comments here and in the rejected #3266, I had compiled this list of examples/test cases for desired unit checking behaviour involving literals: https://gist.github.com/bilderbuchi/317bc1a2e039f477462a61f5826260ea#file-mcp-0027-test-examples-md AFAIK, this list is uncontentious w.r.t. the rejected/accepted outcome of a given example, and probably also w.r.t the given reasoning. It might come in handy during potential future iterations on this MCP's topic.

casella commented 1 year ago

AFAIK, this list is uncontentious w.r.t. the rejected/accepted outcome of a given example, and probably also w.r.t the given reasoning. It might come in handy during potential future iterations on this MCP's topic.

Thanks @bilderbuchi, it is definitely good for me.

mestinso commented 1 year ago

From the various comments here and in the rejected #3266, I had compiled this list of examples/test cases for desired unit checking behaviour involving literals: https://gist.github.com/bilderbuchi/317bc1a2e039f477462a61f5826260ea#file-mcp-0027-test-examples-md AFAIK, this list is uncontentious w.r.t. the rejected/accepted outcome of a given example, and probably also w.r.t the given reasoning. It might come in handy during potential future iterations on this MCP's topic.

One comment regarding the ones with pi and gamma: as is, I agree with the "undecided" status. With that said, I feel the following issue should be resolved and unit="1" should be added to pi, gamma, etc.: https://github.com/modelica/ModelicaStandardLibrary/issues/4046

casella commented 1 year ago

One comment regarding the ones with pi and gamma: as is, I agree with the "undecided" status. With that said, I feel the following issue should be resolved and unit="1" should be added to pi, gamma, etc.: modelica/ModelicaStandardLibrary#4046

I absolutely agree with that. But I understand this may also be a subject of debate. I once added unit = "1" to the definition of pi to one of the libraries I am developing (can't remember which one) and I remember some comments from @dietmarw that pointed out some possible drawbacks. @dietmarw, maybe he can give some advice to modelica/ModelicaStandardLibrary#4046 as well.

gwr69 commented 1 year ago

I am confused: Aren't rad the unit of pi? :)

HansOlsson commented 1 year ago

I am confused: Aren't rad the unit of pi? :)

Yes, and no.

One of the underlying issues with the unit-system is that "rad" and "1" are sort of the same. Having "rad" would imply that A=pi*r^2 should have unit "rad.m2"; which is more confusing - so having "1" is preferable. The reason for selecting radians is exactly that it is possible to treat it as unit "1".

I know there are people that claim that they can differentiate between them, but those tricks normally makes everything more confusing.

gwr69 commented 1 year ago

I am confused: Aren't rad the unit of pi? :)

Yes, and no.

One of the underlying issues with the unit-system is that "rad" and "1" are sort of the same. Having "rad" would imply that A=pi*r^2 should have unit "rad.m2"; which is more confusing - so having "1" is preferable. The reason for selecting radians is exactly that it is possible to treat it as unit "1".

I know there are people that claim that they can differentiate between them, but those tricks normally makes everything more confusing.

D'accord and I would be really interested in any reason to not let pi have final unit = "1".

casella commented 1 year ago

As a side note, I've been participating to Modelica Design Meetings for 20 years, and I sometimes thought our discussions had something in common with the scholarly debates of medieval theologians about how many angels would fit on a pin. Though I was impressed by the progress we made over the years, contrary to the motto that "a language should be designed by a person, not by a committee". At least, Modelica came out way better than Ada 😃 .

I guess the discussion on units is particularly fit for this scenario 😅

casella commented 1 year ago

D'accord and I would be really interested in any reason to not let pi have final unit = "1".

@gwr69 on this topic please also check my comment above, specifically on the difference between unit checking and dimensional consistency checking.

My feeling is that dimensional consistency is what we should try to check, and from that point of view "rad" or "1" make no difference, because they are both dimensionless. Proper unit checking and unit inference are a lot more demanding, and I have a strong feeling that they require some understanding of the equations, as the examples in my comment above demonstrate. Maybe we should eventually have our models checked by LLMs? 😅

HansOlsson commented 10 months ago

Issues that we need to consider and decide on (the list may be incomplete):

HansOlsson commented 9 months ago

To discuss:

henrikt-ma commented 9 months ago
  • Are some issues missed in that list?