modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
472 stars 168 forks source link

Modelica.Blocks.Math.Add block's units #4439

Open GarronFish opened 2 months ago

GarronFish commented 2 months ago

In the example:

model AddEx
  Modelica.Blocks.Math.Add
      add;
  Modelica.Blocks.Sources.ContinuousClock continuousClock;
equation 
  connect(continuousClock.y, add.u1);
  connect(continuousClock.y, add.u2);
end AddEx;

When this is simulated in Dymola the units for u1 and u2 are "s" however the output unit of y is not set. I think the reason for this is that k1 and k2 do not have units set. If the unit of k1 was "1" then the units for y would be the same as the units for u1. This behavior would be consistent with Modelica.Blocks.Math.Gain where the unit for k is "1".

beutlich commented 2 months ago

This behavior would be consistent with Modelica.Blocks.Math.Gain where the unit for k is "1".

Note that this will no longer be the case as of 1290058361a4c51cfd101fe95950beefe4067e86.

Here's the updated model as also demonstrated in this commit:

model AddEx2
  Modelica.Blocks.Math.Add add(k1(unit="1"), k2(unit="1"));
    annotation (Placement(transformation(extent={{-20,-10},{0,10}})));
  Modelica.Blocks.Sources.ContinuousClock continuousClock
    annotation (Placement(transformation(extent={{-60,-10},{-40,10}})));
equation 
  connect(continuousClock.y, add.u1) annotation (Line(points={{-39,0},{-32,0},{-32,6},{-22,6}}, color={0,0,127}));
  connect(continuousClock.y, add.u2) annotation (Line(points={{-39,0},{-32,0},{-32,-6},{-22,-6}}, color={0,0,127}));
end AddEx2;
GarronFish commented 2 months ago

Thanks for highlighting this. So in Dymola this means that the user has to go around providing units for more things which is more cumbersome work alternatively unit checking is not being performed. I think the good solution would be if by default units of parameters were "1" unless the parameter is assigned and then the parameter takes the units of the parameter it is assigned to. I will create a separate ticket.

casella commented 2 months ago

In the case of the Gain block I would be hesitant at adding default units, because in general the input and the output of that block could have different units. Ditto for multiplication blocks.

The case of Add is special, though. You can only add quantities which have the same dimensions, so in this specific case I believe it would be fine to add unit="1" as default to the gains.

I only see a corner case with temperatures, as usual. It makes sense to have one input which is a temperature in degC, to which you add a temperature delta in K. In this case, the output should be degC of course. I guess a good enough unit inference logic could figure that out, but I'm not sure. In any case, using unit = "1" on the two gains of Add seems a good idea to me.

@beutlich, @HansOlsson, @henrikt-ma, would you like to comment on that?

henrikt-ma commented 1 month ago

I agree with @casella that Add is special unit = "1" likely makes better sense in this block than others.

However, I think we must also remember why the unit = "1" is no longer present for Gain; https://github.com/modelica/ModelicaStandardLibrary/blob/e4c16e4b5718ace3119691ff896425822e1669b2/Modelica/Blocks/Math.mo#L543

The reason is that having any unit present in the component causes conflicts when one wants to modify the parameter with an expression carrying another unit (unless one also modifies the unit of the parameter in the block component at the same time). That is, setting unit = "1" for k1 and k2 would make it cumbersome to use the block with other units for these parameters. I'd argue that the sensible application of k1 and k2 is to possibly negate the corresponding input, so I'd be fine with unit = "1"; we just need to be aware of the consequences for other applications.

Edit: @casella is of course also right about the problem that unit = "1" causes for models where unit inference would be able to infer another unit based on the input and output of the block.

Regarding temperatures, don't get me started on why it doesn't make sense to have gain parameters in the Add block at all in this case. :)

casella commented 1 month ago

That is, setting unit = "1" for k1 and k2 would make it cumbersome to use the block with other units for these parameters. I'd argue that the sensible application of k1 and k2 is to possibly negate the corresponding input, so I'd be fine with unit = "1"; we just need to be aware of the consequences for other applications.

Of course you can (ab)use the Add block as a generalized weighted sum, but I wouldn't care too much if we are making this less straightforward.

In hindsight, the Add block should have had two Boolean parameters to flip the sign of each input, the current implementation gives too many degrees of freedom. Unfortunately, we can't change that for backwards compatibility, but for sure we can remove some of the unnecessary degrees of freedom by giving a default unit to the two gains.

BTW, if one really wants to abuse the component and use it for a dimensionally weighted sum, they can always change the units of the two gains upon instantiation. We are not making that final 😃