Skip to content

Have final modifier for these rescalings. #4379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Have final modifier for these rescalings. #4379

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Modelica/Fluid/Examples/DrumBoiler.mo
View file
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}})));
Suggested change
Modelica.Blocks.Math.Gain MW2W(final k=1e6)
Modelica.Blocks.Math.Gain MW2W(final k(unit = "W/MW", displayUnit = "1") = 1e6)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed change by @henrikt-ma makes sense to me

Copy link
Contributor Author

@HansOlsson HansOlsson Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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(
Copy link
Contributor

@henrikt-ma henrikt-ma Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If I am not mistaken about final, there is no point putting final before unit when the value modification is already made final.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

transformation(extent={{37,23},{47,33}})));
Modelica.Thermal.HeatTransfer.Celsius.FromKelvin K2degC
annotation (Placement(transformation(extent={{38,49},{48,59}})));