Skip to content

Conversation

henrikt-ma
Copy link
Contributor

Too often, I have been bitten by the hidden gains in blocks such as Integrator and Add, where unit errors are absorbed by inferring unexpected units for these gains with value 1. This PR comes in three parts corresponding to the initial three commits:

  • Introduce a constant to use instead of a literal 1 when one wants a 1 with unit "1".
  • Use the new constant for the default gains where one doesn't want the wildcard effect.
  • Fix detected unit errors where the affected blocks are used.

I must warn that I will soon be off for vacation, and may not be able to respond to feedback until July 7.

@henrikt-ma henrikt-ma requested a review from casella June 10, 2025 16:28
@henrikt-ma henrikt-ma added L: Blocks Issue addresses Modelica.Blocks L: Math Issue addresses Modelica.Math labels Jun 10, 2025
@AHaumer AHaumer self-requested a review June 10, 2025 16:49
@AHaumer
Copy link
Contributor

AHaumer commented Jun 10, 2025

I agree with you on the fact of getting weird units ...
Fine with me to have a constant "one" (1 with unit "1"), but rather dangerous or misleading for integrator and some other.
Let's look at the integrator: der(y) = k*u;
So if u has unit "1" and you expect y to have unit "1" k has to have unit "1/s".
That's the drawback of having signals without units (or having unit "1").
In other cases unit "1" for k is ok:
If u has unit "m/s" and you expect y to have unit "m" k has to have unit "1".
I need some more time to check the usage in control applications where normally we use k = 1/(integral time constant).

@henrikt-ma
Copy link
Contributor Author

By the way, I believe that those removed unitTime were an accidental consequence of the unfortunate period during which the k in Gain was given unit = "1".

@henrikt-ma
Copy link
Contributor Author

So if u has unit "1" and you expect y to have unit "1" k has to have unit "1/s".
That's the drawback of having signals without units (or having unit "1").

To me this is not a drawback, it is precisely how it should be!

@henrikt-ma
Copy link
Contributor Author

I need some more time to check the usage in control applications where normally we use k = 1/(integral time constant).

I recommend taking a look at the bugfixes for PID and LimPID in this PR.

@AHaumer
Copy link
Contributor

AHaumer commented Jun 10, 2025

So if u has unit "1" and you expect y to have unit "1" k has to have unit "1/s".
That's the drawback of having signals without units (or having unit "1").

To me this is not a drawback, it is precisely how it should be!

That's ok (Sometimes I prefer having signals with units).
But in that case integrator.k should have unit "1/s" instead of "1".

@henrikt-ma
Copy link
Contributor Author

henrikt-ma commented Jun 10, 2025

That's ok (Sometimes I prefer having signals with units).
But in that case integrator.k should have unit "1/s" instead of "1".

Right, and the new default is not preventing this. One alternative is to modify the unit along with the value of the gain:

  Modelica.Blocks.Sources.Constant setpoint(k(unit = "m") = 3.0);
  Modelica.Blocks.Math.Feedback feedback;
  Modelica.Blocks.Continuous.Integrator integrator(k(unit = "1/s") = 10.0);
equation
  connect(setpoint.y, feedback.u1);
  connect(feedback.y, integrator.u);
  connect(integrator.y, feedback.u2);

A better alternative in my opinion is to introduce a new time constant:

  parameter Modelica.Units.SI.Time tau = 0.1;
  Modelica.Blocks.Continuous.Integrator integrator(final k = 1 / tau);

Note how the modification of k completely replaces the default modifier with Modelica.Constants.one which doesn't even have the same unit. This would not have been possible if Integrator would have come with k(unit = "1") = 1 as an attempt to avoid the wildcard effect, since a modifier for k alone would not replace the unit.

Edit: Not that I would recommend it, but it is also possible to opt in for the wildcard effect and have the unit of k determined by unit inference (I strongly dislike specifying numeric values which will become associated with units that are not obvious from the immediate vicinity of the literal):

Modelica.Blocks.Continuous.Integrator integrator(k = 10);

The important part of the change in this PR is that when all you want is pure integration and you don't modify k, this wildcard effect is avoided.

@henrikt-ma
Copy link
Contributor Author

I should also mention the relation to modelica/ModelicaSpecification#3688. With that syntax, the definition of Modelica.Constants.one would become obsolete, as one could just write 1'1' instead of referring to the constant. There would also be additional convenient alternatives for how to parameterize the integrator in my example above:

  Modelica.Blocks.Continuous.Integrator integrator(k = 10'1/s');

or

  Modelica.Blocks.Continuous.Integrator integrator(k = 1 / 0.1's');

(Unfortunately, I am not allowed to explain how the syntax can be enabled in Wolfram System Modeler, but let me know if you would like a demonstration.)

@AHaumer
Copy link
Contributor

AHaumer commented Jun 11, 2025

  Modelica.Blocks.Continuous.Integrator integrator(k = 10'1/s');

I like that idea! Additionally, it should be possible to adapt the unit not only textually but also in the graphical menu.

@AHaumer
Copy link
Contributor

AHaumer commented Jun 11, 2025

The proposal looks good to me, but since I'm not a language specialist I'd like to hear the opinion of e.g. @HansOlsson

@henrikt-ma
Copy link
Contributor Author

I like that idea! Additionally, it should be possible to adapt the unit not only textually but also in the graphical menu.

So far so good, but now we must make sure that this side-topic is not growing too big in the discussion of this PR. To be continued in modelica/ModelicaSpecification#3688.

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 L: Math Issue addresses Modelica.Math
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants