Repository navigation
Fix unit inconsistencies caused by 'pi' carrying a unit - #4817
henrikt-ma wants to merge 2 commits into
Conversation
henrikt-ma
added
the
requires Modelica 3.7
|
Just some preliminary comments. We need to first ensures that CI/CD handling can handle the syntax. |
| startTime=startTime) annotation (Placement(transformation(extent={{-160, | ||
| -30},{-140,-10}}))); | ||
| Modelica.Blocks.Sources.Sine sine(amplitude=200, f=50/pi) | ||
| Modelica.Blocks.Sources.Sine sine(amplitude=200, f=50'Hz'/pi) |
There was a problem hiding this comment.
We should be careful with Hertz (just a note on this, but all need to be checked).
Despite being defined as 1/s it shouldn't be used for everything with unit 1/s according to BIPM, but only periodic signals that are something/s.
In this case it seems like a complete mess and 50/pi might be intended to be 100'rad/s'/(2*pi).
There was a problem hiding this comment.
The unit of f is "Hz", so having "Hz" in its modification seems correct to me.
| Modelica.Blocks.Sources.Constant const2(k=1'F'/100/Modelica.Constants.pi) annotation (Placement(transformation(extent={{70,70},{50,90}}))); | ||
| Modelica.Blocks.Sources.Constant const3(k=1'H'/100/Modelica.Constants.pi) annotation (Placement(transformation(extent={{100,50},{80,70}}))); |
There was a problem hiding this comment.
I believe this needs to be considered more, the 100*pi is 50'Hz'2pi, where 50 Hertz is the normal frequency in Europe.
There was a problem hiding this comment.
This is where the constants are used:
connect(const2.y, variableCapacitor.C) annotation (Line(points={{49,80},{40,80},{40,42}}, color={0,0,127}));
connect(const3.y, variableInductor.L) annotation (Line(points={{79,60},{70,60},{70,42}}, color={0,0,127}));
Here, variableCapacitor.C has unit "F", and variableInductor.L has unit "H".
That's as far as the units go. Regarding how to express the value, would you prefer this?
| Modelica.Blocks.Sources.Constant const2(k=1'F'/100/Modelica.Constants.pi) annotation (Placement(transformation(extent={{70,70},{50,90}}))); | |
| Modelica.Blocks.Sources.Constant const3(k=1'H'/100/Modelica.Constants.pi) annotation (Placement(transformation(extent={{100,50},{80,70}}))); | |
| Modelica.Blocks.Sources.Constant const2(k=0.02'F'/(2*Modelica.Constants.pi)) annotation (Placement(transformation(extent={{70,70},{50,90}}))); | |
| Modelica.Blocks.Sources.Constant const3(k=0.02'H'/(2*Modelica.Constants.pi)) annotation (Placement(transformation(extent={{100,50},{80,70}}))); |
There was a problem hiding this comment.
That is one possibility, the other would be something like:
| Modelica.Blocks.Sources.Constant const2(k=1'F'/100/Modelica.Constants.pi) annotation (Placement(transformation(extent={{70,70},{50,90}}))); | |
| Modelica.Blocks.Sources.Constant const3(k=1'H'/100/Modelica.Constants.pi) annotation (Placement(transformation(extent={{100,50},{80,70}}))); | |
| Modelica.Blocks.Sources.Constant const2(k=1'S'/(2*Modelica.Constants.pi*50'Hz')) annotation (Placement(transformation(extent={{70,70},{50,90}}))); | |
| Modelica.Blocks.Sources.Constant const3(k=1'Ohm'/(2*Modelica.Constants.pi*50'Hz')) annotation (Placement(transformation(extent={{100,50},{80,70}}))); |
I could see some explanation along those lines based on resonance circuits etc, but I don't have the domain knowledge to judge whether people normally write (and/or understand) formulas like that or not.
| rotation=270, | ||
| origin={-10,70}))); | ||
| Basic.Inductor inductor1(i(fixed=true), L=0.1/(2*pi)) annotation (Placement( | ||
| Basic.Inductor inductor1(i(fixed=true), L=0.1'H'/(2*pi)) annotation (Placement( |
There was a problem hiding this comment.
Those changes disclose another dilemma that moparser fron deprecated repository https://github.com/modelica-tools/ModelicaSyntaxChecker is still used here while no adequate replacement is in sight or in-place. Therefore we need to address this CI issue beforehand -> #4821.
The default areas and perimeters of the Fluid.Dissipation records, and of the inputs of dp_twoPhaseMomentum_DP, are computed from a diameter written as a plain number, such as pi*0.1^2/4 for an area and pi*0.1 for a perimeter. Since pi carries a unit, these defaults had the unit of pi instead of m2 and m. The diameters are now given as unitful literals, as already done for the same defaults in ModelicaTest.Fluid.Dissipation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
3 participants
This PR fixes unit inconsistencies which are caused by
Modelica.Constants.picarrying a unit. That is, the presence of a unit onpimakes an entire expression such as0.1/(2*pi)have unit, but not the consistent unit for the component it modifies.I am personally convinced that
pishould not have a unit at all (the topic of #4774), which would make this entire PR unnecessary, but fortunately the changes introduced in this PR are not relying onpihaving a unit.