correct usage of units in MSL?

[modelica-trac-importer]

Reported by dzimmer on 27 May 2013 15:18 UTC
The correct usage of units within the MSL shall be checked. Many tools (as Dymola for instance) apply only very relaxed checks for units because otherwise (according to their reports) too many examples in the MSL would break.

Especially, one issue is annoying: Reals of units 1 are treated the same way as Reals with an undefined unit. Hence you can write stuff like this:

model Foo
  Modelica.SIunits.RelativeDensity x;
  parameter Modelica.SIunits.Voltage y;
equation 
  x = y;
end Foo;

without getting a warning in some tools. We suggest that the MSL is corrected that Reals of unit 1 have to comply with unit checks whereas Reals with undefined unit do not have to comply with unit checks. So the following code shall not generate a warning:

model Foo
  Real x; //no unit specified means "" according to the spec.
  parameter Modelica.SIunits.Voltage y;
equation 
  x = y;
end Foo;

This is currently (rightfully) the case in most unit checkers.

We know it is difficult to comply to unit checks since they are not standardized (it is not part of the specification) but when tool developers complain that they cannot improve their checks because otherwise they break standard library code the dog really starts to eat its own tail.

This topic probably requires discussion between the library group and the design group and vendors. Maybe this issue also has been addressed for the new MSL version but we are not aware of it.

---

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

[modelica-trac-importer]

Comment by ahaumer on 28 May 2013 12:29 UTC
partly resolved by 2752557 for Electrical.Machines;
it seems that this issue comes up with control blocks.

[modelica-trac-importer]

Comment by majetta on 29 May 2013 08:06 UTC
In Electrical.Spice3 we often have cases like that one:

temp := cbe/(cbe+in_p.m_transitTimeHighCurrentDensity)

were temp has the unit K and cbe, in_p.m_transitTimeHighCurrentDensity have the unit current. Hence the right site of this assignment has unit 1. Since this unit 1 results from the calculation cbe/(cbe+in_p.m_transitTimeHighCurrentDensity), I see no other possibility as to define cbe and in_p.m_transitTimeHighCurrentDensity as Real. Does anyone has an opinion on that?
Thanks

[modelica-trac-importer]

Comment by sjoelund.se on 29 May 2013 10:17 UTC
Regarding 9954031 (which referenced this ticket), it seems to have introduced new errors:

[lib/omlibrary/Modelica 3.2.1/Math/package.mo:388:3-388:151:writable]Modelica Assert: Vector v={0,0,0} shall be normalized (= v/sqrt(v*v)), but this results in a division by zero.
Provide a non-zero vector!!

At least the following OpenModelica tests fail from yesterday's MSL changes:

	./flattening/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Systems.RobotR3.fullRobot.mos
	./simulation/libraries/3rdParty/Exercises/ServoSystem3.Aufgabe4_2.mos
	./simulation/libraries/3rdParty/Exercises/ServoSystem3.Aufgabe4_3a.mos
	./simulation/libraries/3rdParty/Exercises/ServoSystem3.Aufgabe4_3b.mos
	./simulation/libraries/3rdParty/Exercises/ServoSystem3.Aufgabe4_4a.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.ForceAndTorque.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.PendulumWithSpringDamper.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.PointGravity.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.PointGravityWithPointMasses2.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.RollingWheel.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.RollingWheelSetDriving.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.RollingWheelSetPulling.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.SpringDamperSystem.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.SpringMassSystem.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.SpringWithMass.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Elementary.Surfaces.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Loops.EngineV6_analytic.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Rotational3DEffects.ActuatedDrive.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Rotational3DEffects.GearConstraint.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Rotational3DEffects.GyroscopicEffects.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Rotational3DEffects.MovingActuatedDrive.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Systems.RobotR3.Components.MechanicalStructure.mos
	./simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Systems.RobotR3.fullRobot.mos

[modelica-trac-importer]

Comment by fcasella on 29 May 2013 11:01 UTC
Replying to [comment:2 majetta]:

In Electrical.Spice3 we often have cases like that one:

temp := cbe/(cbe+in_p.m_transitTimeHighCurrentDensity)

were temp has the unit K and cbe, in_p.m_transitTimeHighCurrentDensity have the unit current. Hence the right site of this assignment has unit 1. Since this unit 1 results from the calculation cbe/(cbe+in_p.m_transitTimeHighCurrentDensity), I see no other possibility as to define cbe and in_p.m_transitTimeHighCurrentDensity as Real. Does anyone has an opinion on that?

I had a look at the code of bjtNoBypassCode where this assignment is, it looks like a one-to-one translation of FORTRAN code, where typically unit checking is not a concern and all variables are just double precision floats. In the field of fluid systems you often see FORTRAN code like this:

P = P * 1e5
....
P = P * 1e-5

explanation: first convert a pressure from bar to Pascal, do the math in SI units, then convert back to bar because the interface is in bar. Obviously it doesn't make sense at all to apply dimensional analysis to this kind of procedural code.

Regarding the code you specifically mention, I see the following lines in the function's body:

temp  := cbe / (cbe + in_p.m_transitTimeHighCurrentF);
argtf := argtf * temp * temp;
arg2  := argtf * (3-temp-temp);

this only makes sense if temp is a nondimentional factor, so it cannot definitely have unit "K". Maybe the name "temp" stands for temporary variable (in the original Fortran code) and not for temperature.

On the other hand m_transitTimeHighCurrentDensity seems closer to a current, but maybe it is a current density [A/m2] or [A/cm2], I'm not sure.

My conclusion would be that if we include in the MSL some functions which are taken from trusted 3rd party FORTRAN codes (such as SPICE), we should probably forget about unit checking entirely inside algorithms, and only make sure we define proper units at the function interface (and that the code is correctly translated from FORTRAN to Modelica). Unless we want to find bugs in the SPICE code, but I guess this is out of the scope of MSL development

[modelica-trac-importer]

Comment by sjoelund.se on 29 May 2013 11:09 UTC
Replying to [comment:4 fcasella]:

My conclusion would be that if we include in the MSL some functions which are taken from trusted 3rd party FORTRAN codes (such as SPICE), we should probably forget about unit checking entirely inside algorithms, and only make sure we define proper units at the function interface (and that the code is correctly translated from FORTRAN to Modelica). Unless we want to find bugs in the SPICE code, but I guess this is out of the scope of MSL development

So just use Real (unit="") instead of Temperature or whatever inside functions in order to escape unit checking (when it is not feasible to write the algorithm section using proper units)?

[modelica-trac-importer]

Comment by hansolsson on 29 May 2013 11:26 UTC
Replying to [comment:2 majetta]:

In Electrical.Spice3 we often have cases like that one:

temp := cbe/(cbe+in_p.m_transitTimeHighCurrentDensity)

were temp has the unit K and cbe, in_p.m_transitTimeHighCurrentDensity have the unit current. Hence the right site of this assignment has unit 1. Since this unit 1 results from the calculation cbe/(cbe+in_p.m_transitTimeHighCurrentDensity), I see no other possibility as to define cbe and in_p.m_transitTimeHighCurrentDensity as Real. Does anyone has an opinion on that?

Well, if cbe and n_p.m_transitTimeHighCurrentDensity both have unit="A"and temp has unit="1" it is also unit-consistent, and to me it looks better to keep that.

If temp is intended to be a temperature the solution would be:

  final constant Real unitToTemp(unit="K")=1;
equation
  temp=unitToTemp*cbe/(cbe+in_p.m_transitTimeHighCurrentDensity);

Yes, as any other cast there is a risk that it was added without understanding the issue, and obviously this only works if temp has the same unit the entire algorithm (which means that we otherwise need to split it into different variables - the Fortran code should have used equivalence statements!)

Thus the question is if temp should have unit="1" or temp has unit="K", but argtf is the temporary that changes units, e.g. on the line:

  argtf := argtf * temp * temp;

[modelica-trac-importer]

Comment by fcasella on 29 May 2013 11:39 UTC
My point is that in order to add units to this SPICE code, we need some amount of reverse engineering, because the original code has no unit information (and maybe doesn't even work with SI units). If that is possible easily without changing the code in the algorithm (which should be as close to the original as possible, if it is trusted), why not. But I would not waste time to introduce things like unitToTemp, which would make a fairly obscure procedural code even more obscure.

My two cents.

[modelica-trac-importer]

Comment by majetta on 29 May 2013 11:48 UTC
Yes, it is true. The Spice3 models are taken from the original SPICE code which does not have SI units. Therefore we planned to add units only in the highest level of the models were users can see them. Inside the functions we planned to use Real, because it is really difficult so figure out the SI units. Unfortunatelly in #414 we were asked to add SI units in the complete Spice3 library.

[modelica-trac-importer]

Comment by otter on 29 May 2013 13:33 UTC
Replying to [comment:3 sjoelund.se]:

Regarding 9954031 (which referenced this ticket), it seems to have introduced new errors:

[lib/omlibrary/Modelica 3.2.1/Math/package.mo:388:3-388:151:writable]Modelica Assert: Vector v={0,0,0} shall be normalized (= v/sqrt(v\*v)), but this results in a division by zero.
Provide a non-zero vector!!

At least the following OpenModelica tests fail from yesterday's MSL changes:

...

Fixed in a8b0e5f (sorry).

[modelica-trac-importer]

Comment by dietmarw on 29 May 2013 16:03 UTC
Shouldn't the newly introduced function Modelica.SIunits.Conversions.to_unit1 follow the MSL naming guidelines, as in use _ only if it can be interpreted as a subscript? Otherwise some code rendering will display this as:

to_unit1

I would suggest to change the name to toUnit1.

[modelica-trac-importer]

Comment by otter on 29 May 2013 20:28 UTC
Replying to [comment:10 dietmarw]:

Shouldn't the newly introduced function Modelica.SIunits.Conversions.to_unit1 follow the MSL naming guidelines, as in use _ only if it can be interpreted as a subscript? Otherwise some code rendering will display this as:

to_unit1

I would suggest to change the name to toUnit1.

All function names under Modelica.SIunits.Conversion (about 30) use the naming convention to_XXX or from_XXX. It looks odd, if one of the function names looks differently.

[modelica-trac-importer]

Comment by majetta on 30 May 2013 05:36 UTC
solved in fc9e116 for Electrical.Spice3 according to the proposal of francesco. Changed some SI unit variables into Real.

[modelica-trac-importer]

Comment by otter on 31 May 2013 17:02 UTC
Fixed in e4d313b for Modelica.Blocks
Fixed in 9954031 and a8b0e5f for Modelica.Mechanics.MultiBody
Fixed in 77bd281 for Modelica.Fluid and Modelica.Media

[modelica-trac-importer]

Comment by otter on 21 Jun 2014 17:54 UTC
It seems in MSL 3.2.1 all units are correct

[modelica-trac-importer]

Comment by beutlich on 16 Mar 2015 09:54 UTC
I guess milestone should be reset to milestone:MSL3.2.1 where changes went. Nothing was changed here for milestone:MSL3.2.1+build.3.

[modelica-trac-importer]

Comment by beutlich on 22 Jul 2015 22:25 UTC
I guess milestone should be reset to milestone:MSL3.2.1 where changes went. Nothing was changed here for milestone:MSL3.2.1+build.3.

[modelica-trac-importer]

Modified by dietmarw on 23 Jul 2015 06:34 UTC