Skip to content
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.

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

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

Open
GarronFish opened this issue Jul 29, 2024 · 5 comments
Open

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

GarronFish opened this issue Jul 29, 2024 · 5 comments
Labels
L: Blocks Issue addresses Modelica.Blocks wontfix Issue that will not be fixed

Comments

@GarronFish
Copy link

GarronFish commented Jul 29, 2024

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
Copy link
Member

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

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;

@beutlich beutlich added wontfix Issue that will not be fixed L: Blocks Issue addresses Modelica.Blocks labels Jul 29, 2024
@GarronFish
Copy link
Author

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

casella commented Jul 31, 2024

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

henrikt-ma commented Aug 5, 2024

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;

parameter Real k(start=1)

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

casella commented Aug 5, 2024

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 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Blocks Issue addresses Modelica.Blocks wontfix Issue that will not be fixed
Projects
None yet
Development

No branches or pull requests

4 participants