Opened 10 years ago

Closed 2 years ago

#2856 closed defect (fixed)

expression simplify affects unit check module

Reported by: lochel Owned by: perost
Priority: high Milestone: 1.19.0
Component: New Instantiation Version: trunk
Keywords: Cc: adrpo, jhagemann@…

Description

There is a new module for unit checking introduced in r22530. It can be enabled with the flag "+newUnitChecking". The module does basically two things:

  1. for all variables unspecified units get calculated if possible
  2. inconsistent equations get reported in a user friendly way

Unfortunately, there is an issue with expression simplify. Please, check the following equation:

  Real u(unit="V");
  Real i(unit="A");
equation
  u = 1 /*Ohm*/ *i;

This equation is consistent, because 1 could have unit "Ohm" but get changed by expressions simplify to the following.

  Real u(unit="V");
  Real i(unit="A");
equation
  u = i;

Now, this equation is inconsistent and get accidently reported by the unit check module.

This can probably be solved in different ways:
Option 1: If the flag +newUnitChecking is used expression simplify get disabled.
Option 2: This module get moved to the front end, before any expression get simplified.
Option 3: ???

Change History (38)

comment:1 Changed 10 years ago by jhagemann@…

even with +noSimplify some simplification are made, since the model fails anyway.

comment:2 Changed 10 years ago by sjoelund.se

I think you need to build up all of these constraints in the front-end and apply a optimizer/solver to find a correct solution.

That is:

  Real u(unit="V");
  Real i(unit="A");
  Real c /* unit=Ohm */;
  Real r(unit="Ohm");
equation
  u = c*i;
  c = r*i;
  u = 1*i;

Add constraints during elaboration of expressions (Types.matchType?):

unit(u)=V
unit(i)=A
unit(u)=unit(c)*unit(i)
unit(c)=unit(r)*unit(i)
unit(u)=unit(tmp1)*unit(i)

And figure out a way to display a nice error :)

Note: We only want to perform unit checking during checkModel. checkModel does not use the backend. So I think a frontend module is more relevant.

comment:3 Changed 10 years ago by anonymous

As an engineer, a university teacher, and a final user of Modelica, I think that an expression
u=1*i;
is to be considered dimensionally erroneous, and totally equivalent to u=i.
Therefore I would recommended to issue a warning, if unit checking is enabled.

So, for me, the behaviour reported in the original message of this ticket is ok.
Under this reasoning, the ticket could be closed without any fix.

comment:4 Changed 10 years ago by sjoelund.se

Well, if you have:

u=(c*r+1)*i

Where r=Ohm, u=V, i=A, and c is a parameter Real c=0 annotation(Evaluate=true);, the expression is simplified into:

u=i;

Which looks like it has the wrong dimensions.

In Modelica, the default unit is "". And the dimensionless unit is "1". They are different. But yes, it does indeed look odd if you can do accept u=1*i. If 1 had unit="1" instead, we could find this error.

But then, what unit would the following get? "" or "1"?

Real c = 1;

Or should there just be warnings for unit="" if the unit is not "1"?

comment:5 Changed 10 years ago by anonymous

I understand that "" as dimension means "unknown" Is it true?
In this case, for me, if unit checking is activated, "" should be considered invalid (i.e. should generate a warning), and dimensionless unit is to be "1".

For me, if i is a current and u is a voltage, the following

u=(c*r+1)*i

is wrong whatever the unit of c and r, because part of u is 1*i that is not a voltage.

In the following expression, instead:

u=c*r*i

if u is in volt, r is in ohm and i in ampere, c must be dimensionless.

comment:6 Changed 10 years ago by lochel

This discussion is actually some kind of off-topic. But anyway, I will try to clarify things.
First of all, there are different approaches for unit checking – basically two:

  1. Each variable, constant, parameter, and number has a unit. Then, unit checking is quite simple and u = 1*i is equivalent to u = i since 1 has no unit (which means unit 1 in this case).
  2. A subset of variables, constants, parameters, and numbers have unit information. All the other need to be determined if possible. Using all this information each expression should get checked for inconsistency. In this case u = 1*i cannot be handled equivalent to u = i because 1 could have any unit (in particular Ohm).

Because the Modelica language does not support to provide unit information for each variable, constant, parameter, and number approach 2 has to be used.

The implemented unit checking works fine. In particular, a lot of msl models are already working but would fail if one would try to apply approach 1. Well, there are still a couple of things missing, but that is just a diligent but routine piece of work.

The problem that need be discussed in this ticket is, how/where should the unit checker take place in the compiler to work on the original expressions. There are also a couple of issues with units/unit checking on my list, but we should try to not mix up different issues in this ticket.

comment:7 follow-up: Changed 8 years ago by Christoph <buchner@…>

lochel: Is there already a way to activate unit checking in OMEdit?
I tried, using OM 1.9.4, adding various variations of --preOptModules+=unitChecking, --preOptModules+=newUnitChecking, etc. to the compiler flags, but it never recognized the option I gave. :-(

comment:8 in reply to: ↑ 7 Changed 8 years ago by lochel

Replying to Christoph <buchner@…>:

lochel: Is there already a way to activate unit checking in OMEdit?
I tried, using OM 1.9.4, adding various variations of --preOptModules+=unitChecking, --preOptModules+=newUnitChecking, etc. to the compiler flags, but it never recognized the option I gave. :-(

I've just tested it, and it works using --preOptModules+=unitChecking.

I used the example UnitCheck1.mos from the testsuite and got the following warning in the message browser of OMedit:

[1] 17:27:34 Translation Warning
The following equation is INCONSISTENT due to specified unit information: V = sin(S + C)
The units of following sub-expressions need to be equal:
- sub-expression "S" has unit "m"
- sub-expression "C" has unit "m/s"

comment:9 Changed 8 years ago by lochel

Maybe you used the wrong place to put the flags. Try Tools -> Options -> Simulation -> OMC Flags.

comment:10 Changed 7 years ago by lochel

  • Component changed from *unknown* to NF - New FrontEnd
  • Milestone changed from Future to 2.0.0
  • Owner changed from somebody to adrpo

comment:11 follow-up: Changed 7 years ago by casella

Lennart clarified the status well in comment:6. And indeed we need literal numbers to have dimension "", not "1", otherwise all binding equations like

SI.Voltage v = 3;

would be dimensionally wrong. That said, I agree with the anonymous comment:5 that writing

u = 4 * i;

where 4 is to be interpreted as 4 Ohms is bad modelling style. One should write

R = 4;
u = R * i;

which would then allow full unit checking.

A related issue about approach number 2 in comment:6 is that it makes it impossible to unit check equations that contain nondimensional numerical factors. For example

v = sqrt(2*g*h);

cannot be dimensionally checked, because the factor 2 would add as a slack dimension and absorb any error. For example, if I forgot the sqrt()

v = 2*g*h

I would not get any error by unit checking, because the factor 2 would get the "right" (i.e., wrong) dimension [s/m] to balance the equation dimensionally. One way to handle this is to write

Real two(unit="1") = 2;
v = sqrt(two*g*h):

which is not really an option.

My proposal is instead to interpret literal constants as having unit "" in binding equations, but to assume they have unit "1" in equation sections. This would

  • enforce a better modelling style
  • ensure tighter unit checking by avoiding the slack dimension syndrome stated above
  • not break any existing MSL model

This should probably be better clarified on the Modelica specification.

What do you think?

comment:12 Changed 7 years ago by lochel

I agree. I think it's quite helpful if Modelica would allow to add unit information to literal constants. Otherwise, unit checking is quite limited in practical usage.

comment:13 follow-up: Changed 7 years ago by lochel

Francesco, I don't think that your approach would work, because it could come into conflicts with the description string of elements. However, we should discuss this further within a Modelica ticket.

Edit: Sorry, I misunderstood your proposal. Does it mean that an equation u = 1e4*i would be detected as having wrong unit information?
Anyway, we should discuss this further within a Modelica ticket.

Last edited 7 years ago by lochel (previous) (diff)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by casella

Replying to lochel:

Edit: Sorry, I misunderstood your proposal. Does it mean that an equation u = 1e4*i would be detected as having wrong unit information?

Absolutely! When you are writing physical equations, numbers should be nondimensional.

Anyway, we should discuss this further within a Modelica ticket.

Sure. Shall I open it?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by lochel

Replying to casella:

Absolutely! When you are writing physical equations, numbers should be nondimensional.

I prefer a solution that allows it to define unit information for literal constants, e.g. u = 1e4 'Ohm' * i.

Sure. Shall I open it?

Yes, please do so.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by sjoelund.se

Replying to lochel:

Replying to casella:

Absolutely! When you are writing physical equations, numbers should be nondimensional.

I prefer a solution that allows it to define unit information for literal constants, e.g. u = 1e4 'Ohm' * i.

You can do that already. Just define final constant SIunits.Resistance Ohm=1.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 7 years ago by lochel

Replying to sjoelund.se:

You can do that already. Just define final constant SIunits.Resistance Ohm=1.

Good point. However, it would not solve the issue, because 1e4 would still have no unit information. Thus, a unit checking algorithm would still not be able to check the equation reliably, as Francesco pointed out in comment 11.

comment:18 in reply to: ↑ 11 Changed 7 years ago by ceraolo

Replying to casella:

Lennart clarified the status well in comment:6. And indeed we need literal numbers to have dimension "", not "1", otherwise all binding equations like

SI.Voltage v = 3;

would be dimensionally wrong. That said, I agree with the anonymous comment:5 that writing

u = 4 * i;

where 4 is to be interpreted as 4 Ohms is bad modelling style. One should write

R = 4;
u = R * i;

which would then allow full unit checking.

A related issue about approach number 2 in comment:6 is that it makes it impossible to unit check equations that contain nondimensional numerical factors. For example

v = sqrt(2*g*h);

cannot be dimensionally checked, because the factor 2 would add as a slack dimension and absorb any error. For example, if I forgot the sqrt()

v = 2*g*h

I would not get any error by unit checking, because the factor 2 would get the "right" (i.e., wrong) dimension [s/m] to balance the equation dimensionally. One way to handle this is to write

Real two(unit="1") = 2;
v = sqrt(two*g*h):

which is not really an option.

My proposal is instead to interpret literal constants as having unit "" in binding equations, but to assume they have unit "1" in equation sections. This would

  • enforce a better modelling style
  • ensure tighter unit checking by avoiding the slack dimension syndrome stated above
  • not break any existing MSL model

This should probably be better clarified on the Modelica specification.

What do you think?

That is a fantastic proposal for me.
It improves readability and quality of Modelica language, bringing it to a higher level, indeed.

comment:19 in reply to: ↑ 17 ; follow-up: Changed 7 years ago by sjoelund.se

Replying to lochel:

Replying to sjoelund.se:

You can do that already. Just define final constant SIunits.Resistance Ohm=1.

Good point. However, it would not solve the issue, because 1e4 would still have no unit information. Thus, a unit checking algorithm would still not be able to check the equation reliably, as Francesco pointed out in comment 11.

1e4 should have a unit of "1", I suppose ;)

comment:20 in reply to: ↑ 19 Changed 7 years ago by lochel

Replying to sjoelund.se:

1e4 should have a unit of "1", I suppose ;)

I see! That would work, I guess. :D

comment:21 follow-up: Changed 7 years ago by casella

I am going to open a ticket on trac.modelica.org on the topic, and eventually prepare an MCP. I'll post it here first for your comments.

Summary: Literal constants in equation sections should have unit "1".

The Modelica Specification 3.3r1 does not define what is the unit attribute of literal Real constants. Both Dymola and OpenModelica implicitly assume that the unit is ""; this means that the unit of the constant can be inferred from the equation it appears in. This is perfectly reasonable for binding equations such as

parameter Modelica.SIunits.Heigth h = 3;

in this case the literal constant is inferred to have unit "m", which is precisely what one would expect from such a declaration. However, this is not a good idea for physical equations inside an equation section. Consider this example, based on the well known Torricelli's law

  Modelica.SIunits.Velocity v;
  Modelica.SIunits.Acceleration g = 9.80665;
  Modelica.SIunits.Height h;
equation
  v = sqrt(2*g*h);

In this case, with the correct equation, the literal constant 2 is inferred to have the unit "1". However, if one writes a wrong equations such as

  v = 2*g*h;

dimensional analysis ends up inferring the unit of 2 to be "s/m" and declaring the equation as correct, which is obviously nonsense. In this context, the unit of 2 should be implicitly assumed to be "1", and consequently unit checking should give an error, which is what already happens if the term 2 is missing as in

  v = g*h;

If one wanted to write literal physical constants in the equation section, the right way to do it in order to end up with a dimensionally consistent equation would then be

  Modelica.SIunits.Voltage v;
  Modelica.SIunits.Current i;
  Modelica.SIunits.Resistance Ohm = 1;
equation
  v = 4*Ohm*i;

I would then propose to add the following normative text to Section 2.4.1: "A literal floating point constant is implicitly assumed to have unit "" if it appears in a binding equation, or to have unit "1" if it appears in an equation or algorithm section.

Non-normative text along the lines of what stated above could be added to explain the rationale.

comment:22 Changed 7 years ago by casella

An even better alternative could be to restrict the case of implicitly assumed unit being "" to equations of the form <variable> = <literal constant>, where it is obvious that unit checking makes no sense, while unit inference is the expected behaviour. In this case the proposal could be along these lines:

A literal floating point constant is implicitly assumed to have unit "" if it appears in an equation of the form <variable> = <literal constant> (including binding equations), otherwise it is assumed to have unit "1".

In fact, this makes more sense, as it would allow unit checking in cases such as

parameter Modelica.SIunits.Height h = 3;
constant Modelica.SIunits.Acceleration g = 9.80665;
final parameter Modelica.SIunits.Velocity v = sqrt(2*g*h);

comment:23 Changed 7 years ago by sjoelund.se

Yes, comment:22 makes more sense. It would perhaps also make sense to have:

  parameter Modelica.SIunits.Height h(fixed=false);
initial equation
  h = 3;
Last edited 7 years ago by sjoelund.se (previous) (diff)

comment:24 Changed 7 years ago by lochel

Good proposal. I'm also in favour of the version of comment:22.

comment:25 follow-up: Changed 7 years ago by ceraolo

I support the proposal in comment 22.
I also appreciate the possibility (shown in comment 19 and never thought of earlier) to make a special Modelica code such as

final constant SIunits.Resistance ohm=1;
Modelica.SIunits.Voltage v;
Modelica.SIunits.Current i;
...
v=2.5 ohm*i;

comment:26 in reply to: ↑ 25 ; follow-up: Changed 7 years ago by casella

Replying to ceraolo:

I also appreciate the possibility (shown in comment 19 and never thought of earlier) to make a special Modelica code such as

final constant SIunits.Resistance ohm=1;
Modelica.SIunits.Voltage v;
Modelica.SIunits.Current i;
...
v=2.5 ohm*i;

This would be nice, but it would probably take 4 years of discussion before it makes it to Modelica 3.6. The MA is sometimes a champion at bike shedding, see, e.g. #2122. I bet this ticket will still be open in 2020 :)

If the goal is to have this feature standardized in the language spec, any proposal that implies a change of the syntax is basically suicide. I've tried to come up with one which only requires a clarification on the interpretation of something which is ambiguous in the current specification. IMO, this has much higher chances of being accepted quickly.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 7 years ago by ceraolo

No modification to specifications is needed, once proposal in comment 22 is accepted.

Replying to casella:

Replying to ceraolo:

I also appreciate the possibility (shown in comment 19 and never thought of earlier) to make a special Modelica code such as

final constant SIunits.Resistance ohm=1;
Modelica.SIunits.Voltage v;
Modelica.SIunits.Current i;
...
v=2.5 ohm*i;

This would be nice, but it would probably take 4 years of discussion before it makes it to Modelica 3.6. The MA is sometimes a champion at bike shedding, see, e.g. #2122. I bet this ticket will still be open in 2020 :)

If the goal is to have this feature standardized in the language spec, any proposal that implies a change of the syntax is basically suicide. I've tried to come up with one which only requires a clarification on the interpretation of something which is ambiguous in the current specification. IMO, this has much higher chances of being accepted quickly.

comment:28 in reply to: ↑ 27 ; follow-up: Changed 7 years ago by casella

Replying to ceraolo:

No modification to specifications is needed, once proposal in comment 22 is accepted.

In fact, some addition would be needed.

comment:29 follow-up: Changed 7 years ago by casella

Replying to casella:

A literal floating point constant is implicitly assumed to have unit "" if it appears in an equation of the form <variable> = <literal constant> (including binding equations), otherwise it is assumed to have unit "1".

Further improved proposal:

A literal floating point or integer constant is implicitly assumed to have unit "" if it appears in an equation of the form <variable> = <expr> (including binding equations), where <expr> is an expression containing only literal constants. Otherwise it is assumed to have unit "1".

Rationale: one may want to write short expressions to specify paramtere values. For examplee, a total surface area A could be given as A = 24*0.154, if one has 24 elements each having 0.154 m2 surface. I often use this pattern in my models. As long as only literal constants are used, the unit should be inferred. If I start using parameters, such as in A = 24*L*D, strong unit checking should be performed.

comment:30 in reply to: ↑ 28 Changed 7 years ago by ceraolo

Replying to casella:

Replying to ceraolo:

No modification to specifications is needed, once proposal in comment 22 is accepted.

In fact, some addition would be needed.

What addition you mean?

comment:31 in reply to: ↑ 29 ; follow-up: Changed 7 years ago by sjoelund.se

Replying to casella:

Replying to casella:

A literal floating point constant is implicitly assumed to have unit "" if it appears in an equation of the form <variable> = <literal constant> (including binding equations), otherwise it is assumed to have unit "1".

Further improved proposal:

A literal floating point or integer constant is implicitly assumed to have unit "" if it appears in an equation of the form <variable> = <expr> (including binding equations), where <expr> is an expression containing only literal constants. Otherwise it is assumed to have unit "1".

Rationale: one may want to write short expressions to specify paramtere values. For examplee, a total surface area A could be given as A = 24*0.154, if one has 24 elements each having 0.154 m2 surface. I often use this pattern in my models. As long as only literal constants are used, the unit should be inferred. If I start using parameters, such as in A = 24*L*D, strong unit checking should be performed.

I would word it a bit differently. Perhaps:

Where <expr> is an expression containing only literal constants and arithmetic operators (excluding operator overloading).

Or

A literal floating point value is implicitly assumed to have unit "" if it appears in an equation of the form <variable> = <expr> (including binding equations), where <expr> is an expression that does not contain any unit information. Otherwise it is assumed to have unit "1".

(Note that integers do not have a unit)

comment:32 in reply to: ↑ 31 Changed 7 years ago by casella

Replying to sjoelund.se:

I would word it a bit differently. Perhaps:

Where <expr> is an expression containing only literal constants and arithmetic operators (excluding operator overloading).

That's much nicer, my proposal was quite convoluted.

In fact, it came to my mind that we should cover other cases such as

  Modelica.SIunits.Length L = 3;
  Modelica.SIunits.Length D1;
  final Modelica.SIunits.Length D2 = sqrt(2)*3;
equation
  D1 = sqrt(2)*L;

In the case of D1, we have to be sure that the square root of a number in an expression with non-literals is considered with unit "1". Conversely, in the case of D2 we have to make sure that sqrt(2) is still considered as having unit "". I'll post a further revised proposal trying to include this and your comment.

(Note that integers do not have a unit)

You are right, so we shouldn't worry about them.

comment:33 in reply to: ↑ 21 Changed 7 years ago by casella

Summary: Units of literal Real constants.

The Modelica Specification 3.3r1 does not define what is the unit attribute of literal Real constants. Both Dymola and OpenModelica implicitly assume that the unit is ""; this means that the unit of the constant can be inferred from the equation it appears in. This is perfectly reasonable for equations such as

parameter Modelica.SIunits.Heigth h = 3;

In this case the literal constant is inferred to have unit "m", which is precisely what one would expect from such a declaration. However, this is not a good idea for physical equations including variables or parameters with units. Consider this example, based on the well known Torricelli's law

  Modelica.SIunits.Velocity v;
  Modelica.SIunits.Acceleration g = 9.80665;
  Modelica.SIunits.Height h;
equation
  v = sqrt(2*g*h);

In this case (with the correct equation), the literal constant 2 is inferred to have the unit "1". However, if one writes a wrong equations such as

  v = 2*g*h;

dimensional analysis ends up inferring the unit of 2 to be "s/m" and declaring the equation as correct, which is obviously nonsense. In this context, the constant "2" must be interpreted as a nondimensional factor with unit "1", and consequently unit checking should give an error, which is what already happens if the term 2 is missing as in

  v = g*h;

The right way to write expressions mixing up variables/parameter with literal physical constants in order to end up with a dimensionally consistent equation would then be

  Modelica.SIunits.Voltage v;
  Modelica.SIunits.Current i;
  final constant Modelica.SIunits.Resistance ohm = 1;
equation
  v = 4*ohm*i;

I would then propose to add the following normative text to Section 2.4.1:

A literal floating point constant is implicitly assumed to have unit "" if it appears in an equation of the form <variable|parameter|constant> = <expr>, where <expr> is an expression containing only literal constants, arithmetic operators (excluding operator overloading), and built-in mathematical functions. [Rationale: in the case of a binding equation such

Modelica.SIunits.Velocity = 4;
Modelica.SIunits.Length L = sqrt(2)*3;

the intended behaviour is obviously that the units of literal constants on the right hand side should be inferred in order to match the unit of the left hand side physical variable.].

Otherwise, it is assumed to have unit "1" [Rationale: if a literal constant shows up in an expression which also contains terms with units, then obviously its intended meaning is to represent a nondimensional factor, so unit checking should be enforced by assuming that. For example:

  Modelica.SIunits.Length h;
  Modelica.SIunits.Acceleration g;
  Modelica.SIunits.Velocity v = sqrt(2*g*h);

when unit checking the binding equation for v, the unit of the literal constant 2 should not be inferred to match the left-hand-side unit, but rather set to "1" in order to check the dimensional consistency of the equation. A dimensionally wrong equation such as

  Modelica.SIunits.Velocity v = 2*g*h;

should trigger a error when performing unit checking. If one wants to put a literal physical constant in an expression containing other terms with units, he can use this pattern

  Modelica.SIunits.Voltage v;
  Modelica.SIunits.Current i;
  final constant Modelica.SIunits.Resistance ohm = 1;
equation
  v = 4*ohm*i;

We should also add basic units constants to Modelica.SIunits, e.g.

constant Modelica.SIunits.Voltage volt = 1;
constant Modelica.SIunits.Current ampere = 1;
constant Modelica.SIunits.Resistance ohm = 1;

so that they can be directly used in models, e.g.

  import SI = Modelica.SIunits;
equation
  v = 4*SI.ohm*i;
Last edited 7 years ago by casella (previous) (diff)

comment:34 follow-up: Changed 7 years ago by ceraolo

Nice. In my comment 25 I forgot the asterisk before ohm.
In the Rationale as proposed by Casella, if the same example is used, I recommend to use the SI name for the unit of resistance, which is "ohm", not "Ohm". (as "volt, ampere, etc., not Volt, Ampere...)

comment:35 in reply to: ↑ 34 Changed 7 years ago by casella

Replying to ceraolo:

Nice. In my comment 25 I forgot the asterisk before ohm.

I see. Then I misunderstood your comment, which is instead perfectly fine :)

In the Rationale as proposed by Casella, if the same example is used, I recommend to use the SI name for the unit of resistance, which is "ohm", not "Ohm". (as "volt, ampere, etc., not Volt, Ampere...)

OK. I'll edit comment:33 accordingly

comment:36 Changed 7 years ago by perost

  • Component changed from NF - New FrontEnd to New Instantiation
  • Owner changed from adrpo to perost

Move all tickets from NF - New Frontend to New Instantiation so we don't have two different categories for the same thing.

comment:37 follow-up: Changed 2 years ago by perost

This might not be an issue with the new frontend anymore, since it does unit checking before the simplification phase in order to avoid this issue. Someone who knows more about this issue needs to check though.

comment:38 in reply to: ↑ 37 Changed 2 years ago by casella

  • Milestone changed from 2.0.0 to 1.19.0
  • Resolution set to fixed
  • Status changed from new to closed

Replying to perost:

This might not be an issue with the new frontend anymore, since it does unit checking before the simplification phase in order to avoid this issue.

If that is the case, then the specific problem reported here is indeed fixed.

A long discussion ensued on the units of literals. For that, please check MCP 0027.

Note: See TracTickets for help on using tickets.