Perform unit checking by default only when doing checkModel

[Francesco Casella]

Until unit checking becomes a lot faster than it is now (see #4626), by default only perform unit checking when doing checkModel, not when doing translateModel or simulate.

[Francesco Casella]

Rescheduled to 1.14.0 after 1.13.0 releasee

[Francesco Casella]

@sjoelund.se, you suggested a solution in ticket:4626#comment:6.

Can you please implement it so we can have this in 1.14.0?

Thanks!

[Martin Sjölund]

"Check of Modelica.Mechanics.MultiBody.Examples.Elementary.DoublePendulum completed successfully.
Class Modelica.Mechanics.MultiBody.Examples.Elementary.DoublePendulum has 1302 equation(s) and 1302 variable(s).
952 of these are trivial equation(s)."
"[/home/marsj/OpenModelica/OMCompiler/Compiler/NFFrontEnd/NFUnitCheck.mo:88:5-88:91:writable] Error: Internal error NFUnitCheck.checkUnits: unit check module failed

[Martin Sjölund]

​https://github.com/OpenModelica/OMCompiler/pull/3075 - I won't start it until the error-message is sane though (and hopefully starts working)...

[Francesco Casella]

@perost, unfortunately Mahder is currently out of the loop, so he can't take care of that. Would you mind having a look and checking what's wrong with NFUnitCheck on DoublePendulum?

[Per Östlund]

Replying to casella:

@perost, unfortunately Mahder is currently out of the loop, so he can't take care of that. Would you mind having a look and checking what's wrong with NFUnitCheck on DoublePendulum?

Fixed in c60b9c2. The issue was just that NFUnitCheck.getFunctionName didn't handle record constructors.

[Francesco Casella]

Thanks Per!

[Martin Sjölund]

The testsuite fails:

 "Check of Modelica.Mechanics.MultiBody.Examples.Elementary.PointGravityWithPointMasses2 completed successfully.
 Class Modelica.Mechanics.MultiBody.Examples.Elementary.PointGravityWithPointMasses2 has 2934 equation(s) and 2934 variable(s).
 2352 of these are trivial equation(s)."
-""
+"[Modelica 3.2.2/Mechanics/MultiBody/Parts.mo:240:5-240:59:writable] Warning: The following equation is INCONSISTENT due to specified unit information:  0.0 = fixedTranslation.frame_a.t[2] + fixedTranslation.frame_b.t[2] + (-fixedTranslation.frame_b.f[3]);
+Warning: The units of following sub-expressions need to be equal:
+- sub-expression "fixedTranslation.frame_a.t[2] + fixedTranslation.frame_b.t[2]" has unit "J"
+- sub-expression "-fixedTranslation.frame_b.f[3]" has unit "N"

Which is:

zeros(3) = frame_a.t + frame_b.t + cross(r, frame_b.f);

Which is position*force = energy, right? So the unit checking is wrong or cross performs too many simplifications before unit checking. Shouldn't the unit be propagated more similar to the type anyway?

[Francesco Casella]

Replying to sjoelund.se:

zeros(3) = frame_a.t + frame_b.t + cross(r, frame_b.f);

This equation is an angular momentum balance. All terms are torques (force X length), which has dimensions N.m, or equivalently J. In fact, angular momentum is the cross product of force and displacement, i.e., you take the product of the orthogonal components, so there is actually no work or energy involved, even if Joules show up. That's why N.m is normally used in this case - think of the max engine torque rating, it would totally wrong to think of it as an energy content. Anyway, when checking for dimensional consistency, this doesn't really matter.

I guess the issue here is that the unit of cross(a,b) should be the product of the units of its two arguments a and b. I don't know why unit checking keeps only the dimension of the second one in this case, while the dimension of r is lost, but that's clearly a bug.

@perost, would you mind having a look at this issue?

[Martin Sjölund]

@casella: Most probably because unit checking was implemented as a separate phase. This means if anything is removed earlier on in the instantiation (and it frequently is), the unit checking will not know of it. It would be safer to have implemented it as part of the type checking, building up the constraints as everything was typed.

The function is marked EarlyInline, which probably simplifies the returned expression...

[Francesco Casella]

OK, now I get it. In equation
zeros(3) = frame_a.t + frame_b.t + cross(r, frame_b.f), it turns out r = {1, 0, 0} , and r is marked as structural, so the second element of the 3D array that you reported is evaluated to 1*frame_b.f[3], which then gets simplified to frame_b.f[3]. Of course this is ok from a numerical point of view, but not from a dimensional point of view.

In order to keep things simple, I guess we should carry out unit checking before any constant-evaluation and simplifications are carried out by the NF.

@perost, would this be possible?

[Per Östlund]

Replying to casella:

OK, now I get it. In equation
zeros(3) = frame_a.t + frame_b.t + cross(r, frame_b.f), it turns out r = {1, 0, 0} , and r is marked as structural, so the second element of the 3D array that you reported is evaluated to 1*frame_b.f[3], which then gets simplified to frame_b.f[3]. Of course this is ok from a numerical point of view, but not from a dimensional point of view.

In order to keep things simple, I guess we should carry out unit checking before any constant-evaluation and simplifications are carried out by the NF.

@perost, would this be possible?

If by possible you mean that it could be done, then sure. If you mean if I'm willing to do it now, then not really. The unit checking is currently done on the DAE structure that the NF produces at the very end, so doing it earlier would basically mean having to reimplement the whole thing.

[Francesco Casella]

Replying to perost:

If by possible you mean that it could be done, then sure. If you mean if I'm willing to do it now, then not really. The unit checking is currently done on the DAE structure that the NF produces at the very end
so doing it earlier would basically mean having to reimplement the whole thing.

I see. No need to do that now (or possibly ever). Maybe we could think of a different approach.

It is not really necessary to go through all the simplifications that are normally carried out by the front-end to make the analysis and code generation of the model easier. After all, the goal of checkModel() is to ensure that:

• there are no syntax errors
• there are no look-up failures
• there are no type issues
• the equations are dimensionally consistent
• the model is balanced (if it is not partial)

Other structural issues involving the solvability of the model are beyond the scope of checkModel(). To see them, one should actually build the simulation executable and see what happens.

When checking a model it is of course necessary to constant evaluate those expressions that influence the number of variables and equations in the flattened model, such as

• parameter expressions in array declaration dimensions
• parameter expressions in conditional equations conditions
• parameter expressions influencing the instantiation of conditional components
• (something else I am missing?)

Would it be possible to disable all constant evaluations and simplifications involving structural parameters in the NF, except the above-mentioned ones, when performing checkModel()?

I guess the resulting DAE should then retain all the required information.

[Francesco Casella]

As a further clarification, when doing unit-checking, the NF should still expand operators such as cross(), but refrain from trying to constant-evaluate them. In this case, the DAE will then contain the scalar equation

0 = fixedTranslation.frame_a.t[2] + fixedTranslation.frame_b.t[2] + 
    r*(-fixedTranslation.frame_b.f[3])

which will pass unit checking as all the terms in the addition have unit J, or N.m

[Per Östlund]

@casella: I changed NFEvalConstants so that it's possible to change what it considers to be constant. By default that's constants and structural parameters, but I now set it so that it only evaluates constants and not structural parameters when checkModel is used. This seems to work as intended, and at least the PointGravityWithPointMasses2 model now works with unit checking.

The NF will still evaluate e.g. dimensions of course, this is done as needed in the NF. NFEvalConstants is used to evaluate all constants and structural parameters which were not evaluated by the earlier phases, to make sure there aren't any references to constants or structural parameters left in the model.

I just realized that I applied the same setting for if-branches too, those should probably consider structural parameters as constant regardless or there might be some issues. I'll change that.

[Per Östlund]

Seems Hudson is having a bad day today, I'll push my changes in later when it's feeling better.

[Adrian Pop]

@perost: you just need a bigger hammer :)
​https://test.openmodelica.org/hudson/job/OpenModelica_TEST_PULL_REQUEST/6750/
I think the PR job got confused about the repo so I just wiped the workspace and restarted it.

[Per Östlund]

Replying to adrpo:

@perost: you just need a bigger hammer :)
​https://test.openmodelica.org/hudson/job/OpenModelica_TEST_PULL_REQUEST/6750/
I think the PR job got confused about the repo so I just wiped the workspace and restarted it.

Thanks :) It's been behaving a bit weird today, failing and then working when trying again without any changes.

[Francesco Casella]

Replying to perost:

@casella: I changed NFEvalConstants so that it's possible to change what it considers to be constant. By default that's constants and structural parameters, but I now set it so that it only evaluates constants and not structural parameters when checkModel is used. This seems to work as intended, and at least the PointGravityWithPointMasses2 model now works with unit checking.

Sounds good!

[Francesco Casella]

Replying to perost:

Seems Hudson is having a bad day today, I'll push my changes in later when it's feeling better.

The Gods are against us... :)

[Francesco Casella]

@sjoelund.se, can you please re-trigger ​PR 3075 after the success the PR reported in comment:17, and close the ticket is everything works fine?

Thanks!

[Francesco Casella]

@sjoelund.se, ​#PR 3075 was archived while still open, so it hasn't made it to the master branch.

Can you please resurrect it and push it through in time for 1.14.0?

Thanks!

[Martin Sjölund]

I have resurrected it. There is an error given for a PowerSystems model:

  final parameter SI.Inductance L = (x_s - x_m) * RL_base[2];
  final parameter SI.Inductance L0 = (x_s + 2 * x_m) * RL_base[2];

x_s / x_m are SIpu.Reactance (= Ohm/Ohm) and RL_base is SI.Resistance so we get H = Ohm as units.

[massimo ceraolo]

I've been a power system engineer for decades.
Over time I got convinced that considering a pu value dimensionless is wrong.

It is simple to explain:
A number expressing a physical quantity is always dimensionless because it is always a ratio.
When I say that a current is I=4A I say that I/A=4, i.e. the ratio of the measured and the reference current (which I call ampere) is 4. Note that writing I/A=4 is allowed (and even in some cases recommended) by CGPM.

If instead of taking as reference the SI unit I consider a different current In, say In=2A, I should say I/In=2.
So, to use p.u. is just to use base quantities different than SI's

With my students I don't even use the term p.u., I use "internal base" quantities. I.e. we use a base internal to our problem, not the universal one defined by SI.

I understand that this is not the exact place where to discuss this. But I somehow felt obliged.

After reading comment 23 I came to the conclusion that I should try to write a paper on this, trying also to build a special proposal for Modelica.
I will see if I can do this before the end of July.

[Martin Sjölund]

I have a comment in the PR as well: ​https://github.com/OpenModelica/OpenModelica/pull/240

Basically, the problem seems to be that RL_base is a resistance but RL_base[2] (only) is used as an inductance. It is quite problematic to add your own ad-hoc conversions in Modelica, stating "I know unit checking will fail here. My magic number does this conversion for this domain". In this case a variable with value 1.0 and unit H/Ohm multiplied would do the trick, but I think the unit checker would actually fail on it since it might be evaluated.

[massimo ceraolo]

Replying to sjoelund.se:

In this case a variable with value 1.0 and unit H/Ohm multiplied would do the trick, but I think the unit checker would actually fail on it since it might be evaluated.

I hope you will find some good trick. Comment 24 says that you need tricks because the very concept of pu involving dimensionless voltages, currents, etc. is rotten.
We will see whether it is me that is rotten :-) or I'm right. I will try to explain things through a paper.

[Francesco Casella]

See the discussion in ​PR #240. In a nutshell:

• RL_base is declared with the wrong type - it is a two-dimensional array where the first element is a resistance and the second an inductance.
• Real[2] is used as a type for RL_base in all other places of the library that follow the same pattern
• I would recommend to declare RL_base as Real[2] there as well, see ​PowerSystems PR #43

If this is the only issue with this ticket, I would recommend to merge the resurrected PR 240 at once, so we can finally get unit checking in 1.14.0. This ticket has been open for two years, and I understand the feature is now mature enough.

Otherwise, most users simply don't know that there is a flag to turn it on, so they will make mistakes when writing code that would be easily catched by unit checking, but will go unnoticed instead. This also happened to me several times recently.

I would also recommend that a unit checking error issues a warning, not an error. Nowhere in the Modelica Specification it is said that models containing dimensional inconsistent equations are invalid and should be rejected. This will also allow to merge PR 240 at once, without needing to wait for PowerSystems to be fixed.

[Francesco Casella]

@sjoelund.se, I wrote this test model in the latest nightly build of OMEdit, using v1.14.0-dev-26534-g3ae6d2dc06 (64-bit) with -d=nfAPI,newInst

model M
  Modelica.SIunits.Mass m;
  Modelica.SIunits.Force F;
  Modelica.SIunits.Acceleration a;
equation
  F = m;
end M;

then I clicked on Check Model and I got the following output

Check of M completed successfully.
Class M has 1 equation(s) and 3 variable(s).
1 of these are trivial equation(s).

Obviously this model is dimensionally inconsistent, as the left hand side of the equation has unit "N" and the right hand side has unit "kg". Why isn't this reported?

[Martin Sjölund]

Maybe checkModel in OMEdit ignores translation flags?

I got:

[M.mo:6:4-6:9:writable] Warning: The following equation is INCONSISTENT due to specified unit information:  F = m;
Warning: The units of following sub-expressions need to be equal:
- sub-expression "m" has unit "kg"
- sub-expression "F" has unit "N"

[Francesco Casella]

I now get the expected result

[1] 13:22:38 Translation Warning
[M: 6:3-6:8]: The following equation is INCONSISTENT due to specified unit
information:  F = m; 

[2] 13:22:38 Translation Warning
The units of following sub-expressions need to be equal:
- sub-expression "m" has unit "kg"
- sub-expression "F" has unit "N"

[3] 13:22:38 Scripting Notification
Check of M completed successfully.

[Christoph Buchner <buchner@…>]

Is this working correctly? Using OpenModelica v1.14.0-dev-26678-g84d09a4a27 (64-bit), I had some failures during checkmodel, which I tracked down to the inclusion of Modelica.Media.Interfaces.PartialMedium.BaseProperties.
In fact, when I open that class and do check model, I get:

[1] 15:32:57 Translation Error
[C:/dev/OM64bit/OMCompiler/Compiler/NFFrontEnd/NFUnitCheck.mo: 129:5-129:91]: Internal error NFUnitCheck.checkUnits: unit check module failed

[2] 15:32:57 Scripting Notification
Check of Modelica.Media.Interfaces.PartialMedium.BaseProperties completed successfully.

[Per Östlund]

Replying to Christoph Buchner <buchner@…>:

Is this working correctly?

It seems like the unit checker fails when it encounters an unknown unit, in this particular case bar. The compiler continues whether the unit checking fails or not though, so the error can be ignored.

In ​#412 I've added bar to the list of known units, and I also added a failtrace for unknown units to help us catch such issues.

There are still issues with other models though, such as EngineV6 which uses the units deg and rev/min. We use the SI base units to define units though, so we can't really represent non-SI units such as deg or rev. Maybe lochel knows how that's supposed to work?

[Francesco Casella]

According to Section 19 of the Specification, units expressions are conform to the international standard ISO 1000-1992 "SI units and recommendations for the use of their multiples and of certain other units", which BTW is now withdrawn and replaced by ISO 80000-1:2009, see ​this page.

Now, bar, deg, rev and min are not SI units. I checked with "certain other units" are present in that document: degree is there (with the symbol °), as well as min, but not bar nor rev.

@buchner, which model did you check, exactly? Where is the bar unit appearing?

Regarding deg and min, if we stick to the letter of the specification we should allow it. Maybe we should open a ticket on the Modelica Library trac?

[Christoph Buchner <buchner@…>]

@casella: I checked Modelica.Media.Interfaces.PartialMedium.BaseProperties. As far as I can see (from flattening the model), bar occurs in several places, e.g. Modelica.SIunits.Conversions.to_bar and Modelica.Media.Interfaces.PartialMedium.BaseProperties.p and Modelica.Media.Interfaces.PartialMedium.BaseProperties.p_bar.

Interestingly, checking Modelica.SIunits.Conversions.to_bar completes successfully.

More descriptive error message than "unit check module failed" will help - is the failtrace that you added visible by default in OMEdit, or do I have to add this somewhere (translation flags?) ?

[Per Östlund]

Replying to Christoph Buchner <buchner@…>:

More descriptive error message than "unit check module failed" will help - is the failtrace that you added visible by default in OMEdit, or do I have to add this somewhere (translation flags?) ?

You need to at least add the flag -d=failtrace, and possible --showErrorMessages depending on how OMEdit handles such messages. I added it mainly for my own sake, but we should probably give some proper errors for things like this.

We should probably have someone take a proper look at the unit checking if we're going to have it enabled by default when doing checkModel though, since it's currently failing for many MSL models. I'll bring it up on our next developer meeting on monday.

[Per Östlund]

Replying to perost:

We should probably have someone take a proper look at the unit checking if we're going to have it enabled by default when doing checkModel though, since it's currently failing for many MSL models. I'll bring it up on our next developer meeting on monday.

The outcome of the meeting was that casella will bring the issue up with the MAPLIB group and try to get some clarification on which units should be supported.