Have final modifier for these rescalings. #4379
Conversation
Otherwsise these parameters could (as default; depending on tool) be possible to edit after translation. Modifying the rescaling between Pascal and bar (or MW and W) clearly does not make sense.
| annotation (Placement(transformation(extent={{-54,-75.5},{-44,-64.5}}))); | ||
| Modelica.Blocks.Math.Gain Pa2bar(k=1e-5) annotation (Placement( | ||
| Modelica.Blocks.Math.Gain Pa2bar(final k=1e-5) annotation (Placement( |
There was a problem hiding this comment.
I find it rather obscure to leave it for unit inference to figure out the unit of the gain, and I find setting displayUnit a good way to make clear that the numeric value is the correct one:
| Modelica.Blocks.Math.Gain Pa2bar(final k=1e-5) annotation (Placement( | |
| Modelica.Blocks.Math.Gain Pa2bar(final k(unit = "bar/Pa", displayUnit = "1") = 1e-5) annotation (Placement( |
There was a problem hiding this comment.
(If I am not mistaken about final, there is no point putting final before unit when the value modification is already made final.)
There was a problem hiding this comment.
Why not to use Modelica.Blocks.Math.UnitConversions.To_bar instead of gain?
Similar for K2degC instance which shall be Modelica.Blocks.Math.UnitConversions.To_degC IMO.
There was a problem hiding this comment.
I agree that ideally that should have been done, but:
- I wanted minimal changes in the model
- It should be an example of how to build similar models, and in general there might not exist such blocks for the unit you want to use; so showing that you can just use a gain-block seems like a good idea.
| @@ -70,9 +70,9 @@ package DrumBoiler | |||
| Modelica.Blocks.Interfaces.RealOutput V_l(unit="m3") "Liquid volume inside drum" | |||
| annotation (Placement(transformation(extent={{100,74},{112,86}}))); | |||
| public | |||
| Modelica.Blocks.Math.Gain MW2W(k=1e6) | |||
| Modelica.Blocks.Math.Gain MW2W(final k=1e6) | |||
There was a problem hiding this comment.
Similar to Pa2bar but this should be combined with specifying the unit = "MW" for the table source:
Modelica.Blocks.Sources.TimeTable q_F_Tab(table = [0, 0; 3600, 400; 7210, 400], y(unit = "MW")) if not use_inputs annotation(Placement(transformation(extent = {{-90, -80}, {-70, -60}})));
| Modelica.Blocks.Math.Gain MW2W(final k=1e6) | |
| Modelica.Blocks.Math.Gain MW2W(final k(unit = "W/MW", displayUnit = "1") = 1e6) |
There was a problem hiding this comment.
The proposed change by @henrikt-ma makes sense to me
There was a problem hiding this comment.
I might possibly accept the change of unit, but changing the displayUnit to "1" is "too cute" and a very bad idea.
The reason is that the display unit is used to input (and show) values in the display unit (after a conversion); that is the entire reason for having display units. In normal cases that is useful so that for Real x(unit="W", displayUnit="MW")=1e6; one would input/see "1 MW" (in various ways).
With displayUnit="1" it means that one should input a value in the display unit "1" and then have that converted to "W/MW", or more specifically input the value 1 in the display unit "1". It is cute that that the factor is exactly 1 for a conversion that in some sense doesn't change anything, but unless tools treat such cases specially in the GUI it will seem as if we are converting from "MW" to "W" using a conversion factor 1 (in unit "1").
Seeing a conversion factor 1 for something that clearly should convert between "MW" and "W" is just confusing, and not at all helpful!
And, if we are going to have special rules for such conversions it seems easier to say something "a variable used as multiplicative factor converting between different prefix-variants of the same unit should only do that using the correct factor, and not have any additional factor", or alternatively one could add a helper function for that (such as conversion(1e6, "MW", "W")), instead of having a special rule for not converting and displaying in this specific case. That would fix the factor to the right value, no need for display unit or even unit.
The conversion-function would in one sense just return 1e6 (with unit "W/MW"), but also check that it is the right factor, and it would be clearer that it is just converting - and not some weird correlation (like we lose 20 W in the cables per MW of transmitted power.)
There was a problem hiding this comment.
Seeing a conversion factor 1 for something that clearly should convert between "MW" and "W" is just confusing, and not at all helpful!
Well, we simply disagree here. To me, the conversion factor 1 means that I can directly see that this gain block doesn't scale the quantity. Once you get used to the idea of explicit unit conversion using gain blocks, it becomes completely natural to just specify the target-to-source unit ratio, select display unit "1", and then enter the number 1 without having to think about what is the correct conversion factor. The conversion factor happens to be rather trivial in this case, but generally it can be much harder to immediately see which conversion factor that corresponds to no scaling of the unit-converted quantity.
|
My intent was to not do anything regarding units in this PR, but only make the rescalings final - as they are clearly not intended to be changed. |
There was a problem hiding this comment.
As I see it, it is because the units are final that the conversion rates are final. If the intent is to use these blocks for conversion between other units I don't think it makes sense to make the rates final, so I don't see the point of proceeding without also specifying unit. Maybe you can find another reviewer who likes the changes without unit, and if you do I can make a follow-up PR to only add the missing units?
That makes sense. So can some fluid-experts look at the changes? |
| @@ -70,9 +70,9 @@ package DrumBoiler | |||
| Modelica.Blocks.Interfaces.RealOutput V_l(unit="m3") "Liquid volume inside drum" | |||
| annotation (Placement(transformation(extent={{100,74},{112,86}}))); | |||
| public | |||
| Modelica.Blocks.Math.Gain MW2W(k=1e6) | |||
| Modelica.Blocks.Math.Gain MW2W(final k=1e6) | |||
There was a problem hiding this comment.
The proposed change by @henrikt-ma makes sense to me
| annotation (Placement(transformation(extent={{-54,-75.5},{-44,-64.5}}))); | ||
| Modelica.Blocks.Math.Gain Pa2bar(k=1e-5) annotation (Placement( | ||
| Modelica.Blocks.Math.Gain Pa2bar(final k=1e-5) annotation (Placement( |
Otherwise these parameters could (as default; depending on tool) be possible to edit after translation.
Modifying the rescaling between Pascal and bar (or MW and W) clearly does not make sense.