[henrikt-ma]

This PR fixes unit inconsistencies which are caused by Modelica.Constants.pi carrying a unit. That is, the presence of a unit on pi makes an entire expression such as 0.1/(2*pi) have unit, but not the consistent unit for the component it modifies.

I am personally convinced that pi should 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 on pi having a unit.

[HansOlsson]

Just some preliminary comments. We need to first ensures that CI/CD handling can handle the syntax.

[HansOlsson]

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).

[henrikt-ma]

The unit of f is "Hz", so having "Hz" in its modification seems correct to me.

[HansOlsson]

I believe this needs to be considered more, the 100*pi is 50'Hz'2pi, where 50 Hertz is the normal frequency in Europe.

[henrikt-ma]

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?

Suggested change

	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}})));

[HansOlsson]

That is one possibility, the other would be something like:

Suggested change

	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.

[HansOlsson]

#4774 (comment) gave a more readable solution

[henrikt-ma]

So the suggestion is simply to introduce new parameters instead of the unitful literals? If this is what everyone wants, I'll do it that way. @AHaumer ?

[HansOlsson]

To be clear it is new parameters revealing the logic of the model, as indicated in that comment.

[henrikt-ma]

I think the suggestion in "that comment" is missing some use of final. @AHaumer would you agree with this?

  parameter SI.Frequency fNom=50;
  parameter SI.Admittance Y=1;
  final parameter SI.Capacitance C=Y/(2*pi*fNom);
  parameter SI.Reactance X=1;
  final parameter SI.Inductance L=X/(2*pi*fNom);
  Modelica.Blocks.Sources.Constant const2(final k=C);
  Modelica.Blocks.Sources.Constant const3(final k=L);

Or should some of {fNom, Y, X} also be final?

(ParallelResonance and SeriesResonance would make similar use of final, I suppose.)

[beutlich]

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.

[HansOlsson]

See #4774 (comment) for a better alternative for this model

[HansOlsson]

This also indicates that the classes aren't well-designed.

Both the cross area and perimeter have defaults based on the diameter being 0.1'm', but instead of having one default depending on the other input (or on a common diameter input), the default diameter is duplicated in this function and related ones (it can only be a default as the goal is to support non-circular cross-sections). That makes it more error-prone.

It would be possible to introduce a backwards compatible change for that by having a new trailing diameter-input that only impacts the defaults. The down-side is that existing calls wouldn't modify the default-diameter and that value would be a bit confusing.

Alternatively, it would also be possible to have the default for the cross-section depend on the perimeter (A_cross=perimeter^2/(4*PI) without that down-side. Technically it would be non-backwards compatible, but any impacted model would be problematic.

(Note that it uses the general term "perimeter", not "circumference".)

[AHaumer]

Sorry, no I won't accept what is discussed here (and in other PRs touching that topic).
pi might be an angle, having the unit "rad" equivalent to "1".
pi might be just an irrational mumber, having no unit.
Tools should develop heuristics to decide whether to use "no unit" or "rad".
I will also not accept changes in the MSL like:
Modelica.Blocks.Sources.Constant const2(k=1'S'/(2*Modelica.Constants.pi*50'Hz'));
That's horrible! Just spend 2 parameters and it looks nice:
parameter SI.Conductance Y=1;
parameter SI.Frequency f=50;
Modelica.Blocks.Sources.Constant const2(k=Y/(2*pi*f));