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

Unit-checking using Gain and LimIntegrator-blocks #3878

Closed
HansOlsson opened this issue Oct 11, 2021 · 13 comments · Fixed by #3881
Closed

Unit-checking using Gain and LimIntegrator-blocks #3878

HansOlsson opened this issue Oct 11, 2021 · 13 comments · Fixed by #3881
Labels
bug Critical/severe issue L: Blocks Issue addresses Modelica.Blocks L: Electrical.Batteries Issue addresses Modelica.Electrical.Batteries
Milestone

Comments

@HansOlsson
Copy link
Contributor

Currently Dymola as default sort of ignores unit="1"; and thus the change in #227 is not yet active.

Trying to activate that check triggers warnings in several models, some can be traced to Modelica.Electrical.Batteries.BaseClasses.BaseCellStack ( https://github.com/modelica/ModelicaStandardLibrary/blob/3057a7665290da6111c62c04ec40e29c9c8c069b/Modelica/Electrical/Batteries/BaseClasses/BaseCellStack.mo and similarly BaseCellStackWithSensors in the same package) where

Blocks.Continuous.LimIntegrator limIntegrator(
    final k=1/(Np*cellData.Qnom),
   ...
...
Modelica.Blocks.Math.Gain gainV(final k=Ns*cellData.OCVmax)

and cellData.Qnom.unit="A.s", and cellData.OCVmax.unit="V".

I assume we want the improved unit-checking, and that in most cases unit="1" is correct, so we should change these uses - and the simplest correction in that case is to add a modifier with unit="" and just let normal unit-deduction work out the result.

That gives k(unit="")=1/(Np*cellData.Qnom), and we can provide that as a recommendation for users.
The other alternative is k(unit="s-1.A-1")=1/(Np*cellData.Qnom), but that just puts the burden on the user of the library and doesn't add anything meaningful.

I can provide a PR for it.

Also in Modelica.Electrical.Machines.Examples.DCMachines.DCPM_CurrentControlled

@HansOlsson HansOlsson added bug Critical/severe issue L: Blocks Issue addresses Modelica.Blocks L: Electrical.Batteries Issue addresses Modelica.Electrical.Batteries labels Oct 11, 2021
HansOlsson added a commit to HansOlsson/Modelica that referenced this issue Oct 11, 2021
@beutlich
Copy link
Member

beutlich commented Oct 11, 2021

unit="" seems like a new concept to remove unit modifiers in favour of automatic unit-deduction. It currently is not present in MSL (master).

I am rather in favour to fix the actual unit of the gain. For example, in Modelica.Electrical.Batteries.BaseClasses.BaseCellStack.limIntegrator it is u.unit = "A" and y.unit = "". Thus, k.unit should be "A-1"? Maybe, the unit of SoC in Batteries should be changed to "1" as well. @AHaumer @christiankral

See also:

Unfortunately, it is called unitResistance in one place but oneOhm in another. But that is a different story.

@HansOlsson
Copy link
Contributor Author

unit="" seems like a new concept to remove unit modifiers in favour of automatic unit-deduction. It currently is not present in MSL (master).

It was implicitly present in those block prior to #227.

I am rather in favour to fix the actual unit of the gain. For example, in Modelica.Electrical.Batteries.BaseClasses.BaseCellStack.limIntegrator it is u.unit = "A" and y.unit = "". Thus, k.unit should be "A-1"? Maybe, the unit of SoC in Batteries should be changed to "1" as well. @AHaumer @christiankral

At first I was also considering that for the specific cases, but then I realized that this is also a general issue where we need to give a guideline for users.

That's when I realized that deducing the unit for k is just tedious work - you look at the formula for k and see what unit it has and put it in the unit-field; whereas if you set k(unit="")=1/Qnom you allow the tool to do that; so the goal was to have the general recommendation for these blocks - if you are using a formula with unit use the syntax k(unit="")=... to ensure that the unit for k is given by ..., which to me is a general easily understandable formula. Using k(unit=break)=... after MCP-0009 is also a possibility.

I'm not saying that giving the specific unit is wrong - just that it tedious and gains very little; but we can still state that it is an alternative.

BTW: I think the reason you got k.unit="A-1" instead of "A-1.s-1" is that you missed that the integrator also integrates.

@henrikt-ma
Copy link
Contributor

I find this an interesting discussion about a largely unspecified topic. One question I find particularly interesting is whether a model with a unit error involving the inferred unit of a variable should be considered an invalid model. That is, it would be great if we could agree that the specification should define this as a model error:

  Real a(unit = "m") = 1.0;
  Real b = a;
  Real c(unit = "kg") = b;

To clarify, the alternative interpretation that I would like to be ruled out is that the original empty unit of b is what should be considered in the declaration equation for c.

@HansOlsson
Copy link
Contributor Author

HansOlsson commented Oct 12, 2021

I find this an interesting discussion about a largely unspecified topic. One question I find particularly interesting is whether a model with a unit error involving the inferred unit of a variable should be considered an invalid model. That is, it would be great if we could agree that the specification should define this as a model error:

  Real a(unit = "m") = 1.0;
  Real b = a;
  Real c(unit = "kg") = b;

To clarify, the alternative interpretation that I would like to be ruled out is that the original empty unit of b is what should be considered in the declaration equation for c.

Yes, that would be the intent - but we currently haven't even defined the rules to forbid the simpler

Real a(unit = "m") = 1.0;
Real d(unit = "kg") = a;

Note that MCP-0027, modelica/ModelicaSpecification#2127 has been stuck after starting on such simple cases, since instead of discussing small incremental changes we got side-tracked into redefining everything and trying to make it super-strict.

@christiankral
Copy link
Contributor

I need some clarification here, please.

unit="" seems like a new concept to remove unit modifiers in favour of automatic unit-deduction. It currently is not present in MSL (master).

I have to admit I was not aware of the fact (so far) that using unit="" is a meaningful and desired option in Modelica. If I understand the discussion correctly

  • unit="" indicates that the unit shall be deduced automatically by the tool
  • unit="1" indicates that the unit is equal 1 as in R2 = k * R1 with k(unit="")

I assume this is somewhere written in the specification (which I could not yet find) and consequently the different meaning and intention of unit="" and unit="1" is obviously to all tool vendors. Right?

When looking at the Modelica Language Specification 3.5 I found:

image

So obviously, unit = "" is the default unit of a Real type variable. Why is it then required to use the modifier unit=""?

@HansOlsson
Copy link
Contributor Author

So obviously, unit = "" is the default unit of a Real type variable. Why is it then required to use the modifier unit=""?

Because internally the Gain, Integrator and some other blocks have the modifier unit="1" due to #227, so adding unit="" for specific uses of them gets back to the original unit="", and the logical conclusion is that it should behave the same as the original (MCP-0009 might give another way of doing that).

As noted above one alternative fix for this would be to revert #227 and don't set unit for those blocks - I could make such a PR if that is desired. (There's also the possibility of mixing; but that's probably too messy.)

@HansOlsson
Copy link
Contributor Author

@dietmarw you reported #227 so you might have input on the implications here.

@mestinso
Copy link
Collaborator

I would argue #227 should be rolled back. While, of course, there are situations where the units on a gain block or a gain term in an integrator block have units of "1", I don't think those situations should be assumed by default as I'm not even convinced they are the majority. Better to leave the units blank and then add when the unit="1" situation applies. From a pure downstream use case perspective, the workflow of adding units to gain blocks is much more natural than the workflow of removing units from gain blocks. I'm a bit of a purist, but this is my line of thinking.

HansOlsson added a commit to HansOlsson/Modelica that referenced this issue Oct 14, 2021
If you want unit="1" for a gain-block, add parameter with unit="1" as in the updated test-case.
Closes modelica#3878
@HansOlsson
Copy link
Contributor Author

So I now made #3881 for fixing it in the other way (by reverting #227, and demonstrating adding unit="1" in the test case, and also an additional copy-paste case).
As far as I can tell it checks in Dymola even when being picky about unit="1".

So either #3879 or #3881 should be accepted, in my opinion.

@dietmarw
Copy link
Member

Reading the original #227 and this ticket here it occurs to me that #227 was basically a tool specific hack to silence Dymola at the time. So I'm in favour of reverting #227 again and this in the way as it is done in #3881 since it seems strange to first define unit="1" to then having to revert this explicitly again with unit="" when the latter is the default anyway.

@HansOlsson
Copy link
Contributor Author

Reading the original #227 and this ticket here it occurs to me that #227 was basically a tool specific hack to silence Dymola at the time.

Looking at the model I don't see how it is silencing. If I tried to restore the functionality and log units in Dymola getting:

AsUnitless.u(unit= "A")
Asec.u(unit= "A")
Asec.y(unit= "C")
Asec.y_start(unit= "C")
VSUnitless.u(unit= "V")
Vsec.u(unit= "V")
Vsec.y(unit= "Wb")
Vsec.y_start(unit= "Wb")

So, as expected, setting unit="1" (and activating some flags) gives consistent units to more variables which seems desirable. I don't see any error messages being silenced.

So I'm in favour of reverting #227 again and this in the way as it is done in #3881 since it seems strange to first define unit="1" to then having to revert this explicitly again with unit="" when the latter is the default anyway.

I can still understand this.

@beutlich beutlich added this to the MSL4.1.0 milestone Nov 12, 2021
@ceraolo
Copy link

ceraolo commented Jul 30, 2024

I see that the topic of unit deduction and the presence of unit="1" is Gain is highly debated.
I know that there are advantages and disadvantaged of having this specification in Gain:

  • if it is dropped, output unit deduction is not done, at least in Dymola
  • if it is there, output unit deduction is not done in case the Gain's constant has some dimension, e.g. "s".

However, I see in MSL an issue of consistency. Consider as an example block Add. Its constants k1 ad k2 do not have unit "1", and therefore its output unit is not possible to deduce, at least in Dymola.

This is not consistent. (BTW, I think that is is more common to have unitless k2 and k2 of Add, than k of Gain, since Add is naturally used to add or substract signals, while control gains often have dimensions.)

So, if Dymola's present behaviour is compliant to MS, the situation is inconsistent and, I would say, displaced towards the wrong direction. But discussing whether unit="1" is better put on Gain or Add is subject to personal opinions.

For all this, I think that it would be very useful to drop unit="1" in Gain of MSL. In that case (with the current Dymola behaviour):

  • we would lose automatic equality of Gain output unit to its input, but we would gain the possibility do define gains with k with dimension, with automatic output unit deduction, which would be a boon
  • we would gain consistency with blocks such Add (with all the the blocks I would say, but this should be checked).

Naturally, if this proposal is considered in principle acceptable, the whole list of blocks should be verified and made consistent with it.

@casella
Copy link
Contributor

casella commented Jul 31, 2024

See my last comment in #4439. I also think we should not have unit = "1" in Gain, and instead have it in Add.

Units are complicated...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Critical/severe issue L: Blocks Issue addresses Modelica.Blocks L: Electrical.Batteries Issue addresses Modelica.Electrical.Batteries
Projects
None yet
8 participants