modelica
Added "four-line" proposal for units of literals
Here is my concrete proposal for MCP/0027. The normative text is a few lines (not sure if four or how many), I also added a bit longer description of the rationale in the non-normative text, to clarify why it is formulated in that way.
It seems like this proposal has a similar shortcoming as in #3257 when it comes to allowing for connector variable unit inference. I think both proposals should to be modified to not rule out such unit inference.
For example, I expect that the literal in the following model is expected go get unit "1", even though u has unit "":
block Double
Modelica.Blocks.Interfaces.RealInput u;
Modelica.Blocks.Interfaces.RealOutput y;
equation
y = 2 * u;
end Double;
My main concern is that we seem to have a number of models where numbers are assumed to have a unit, e.g. Modelica.Electrical.Batteries.Examples.BatteryDischargeCharge where Ri=cellData1.OCVmax/1200 (the 1200 is assumedly a current), or Modelica.Electrical.Machines.Utilities.ParameterRecords.IM_SquirrelCageData, and all of the Modelica.Electrical.Spice3.Internal.Mos formulas.
There are also some cases where people write numbers such as 0.0289651159 with an implicit unit (I assume you get it Francesco).
It seems like this proposal has a similar shortcoming as in #3257 when it comes to allowing for connector variable unit inference. I think both proposals should to be modified to not rule out such unit inference.
For example, I expect that the literal in the following model is expected go get unit
"1", even thoughuhas unit"":block Double Modelica.Blocks.Interfaces.RealInput u; Modelica.Blocks.Interfaces.RealOutput y; equation y = 2 * u; end Double;
I would say that it isn't clear how much of a short-coming it is. We could also write it as model Double=Gain(final k=2); and we recently removed the k.unit="1", https://github.com/modelica/ModelicaStandardLibrary/pull/3881 and I wouldn't be surprised that some use k with a literal value and a non-1 unit.
Separating integers and reals seems tempting for 2 and 2.0, but 101325 and 299792458 are also integers.
My main concern is that we seem to have a number of models where numbers are assumed to have a unit, e.g. Modelica.Electrical.Batteries.Examples.BatteryDischargeCharge where
Ri=cellData1.OCVmax/1200(the 1200 is assumedly a current), or Modelica.Electrical.Machines.Utilities.ParameterRecords.IM_SquirrelCageData, and all of the Modelica.Electrical.Spice3.Internal.Mos formulas.
I find it concerning that the MSL contains such formulas. I think it is the job of the specification to point of the need for 1200 to get a proper unit.
Clearly, we need to introduce unit checking in the specification with some sort of transition in mind. The fact that the specification hasn't said anything about unit errors so far means that introducing unit checking should be expected to be a backwards incompatible change. For a smooth transition into a unit-correct world, I think we should count on tools mitigating the effects of the backwards compatibility break. For example, a tool could have a non-strict unit checking mode, where unit errors are diagnosed as warnings. Exactly how to do this for an acceptable user experience should be up to the tools, in my opinion; the specification should call an error by its real name.
I would say that it isn't clear how much of a short-coming it is. We could also write it as
model Double=Gain(final k=2);and we recently removed the k.unit="1", modelica/ModelicaStandardLibrary#3881 and I wouldn't be surprised that some use k with a literal value and a non-1 unit.
My expectation of the Double block would correspond to Gain(final k(unit = "1") = 2). If I wanted a non-unit-preserving relation between input and output, I probably wouldn't have called the block "Double".
I doubt that there's any sensible applications for a Double-like block that without modifying the unit of the gain allows any combination of units on the input and output. In other words, I expect any Double-like block to relate input and output units via some particular unit for the gain. To this end:
- Assume there is unit propagation in modifications; if
khas empty unit in theGain, then it gets an inferred unit when set with a modification. (This is an underlying assumption of https://github.com/modelica/ModelicaStandardLibrary/pull/3881.) - When a component with empty unit (after unit inference has been applied) is used as a component reference expression, that expression gets unit
"1".
With this approach, Gain(final k=2) would actually be equivalent to Gain(final k(unit = "1") = 2) for purposes of unit checking, and it would just be a matter of taste whether to make the "1" explicit or implicit.
It seems like this proposal has a similar shortcoming as in #3257 when it comes to allowing for connector variable unit inference. I think both proposals should to be modified to not rule out such unit inference.
The goal of this proposal is just to expand the scope of dimensional consistency checking. Unit inference is another story.
My main concern is that we seem to have a number of models where numbers are assumed to have a unit, e.g. Modelica.Electrical.Batteries.Examples.BatteryDischargeCharge where
Ri=cellData1.OCVmax/1200(the 1200 is assumedly a current), or Modelica.Electrical.Machines.Utilities.ParameterRecords.IM_SquirrelCageData
Hans, I fully understand your concerns. As far as I am concerned, I find those expressions a bit questionable. You write "the 1200 is assumedly a current". I agree with the assumption, but if the model was written as
...
constant SI.Current i0 = 1200;
...
Ri=cellData1.OCVmax/i0);
it would have been much clearer, and you wouldn't have had to assume anything. This is even more true for Modelica.Electrical.Machines.Utilities.ParameterRecords.IM_SquirrelCageData: I find
parameter SI.Inductance Lm=3*sqrt(1 - 0.0667)/(2*pi*
fsNominal) "Stator main field inductance per phase";
a bit fishy. I mean, the unit of inductance is "H", i.e. "V.s/A". On the RHS I see a term 1/(2*pi*fsNominal) which has a unit "s". I would then guess that 3 means 3 Ohm and sqrt(1 - 0.06667) is some dimensionless correction factor. But again, why not making this explicit as
constant SI.Resistance R0 = 3;
parameter SI.Inductance Lm=R0*sqrt(1 - 0.0667)/(2*pi*fsNominal) "Stator main field inductance per phase";
which could be positively confirmed to be dimensionally kosher if we introduce this proposal in the language spec? I think we can make this argument with @AHaumer and @christiankral and possibly rewrite these modifiers in a more unit-friendly way.
In any case, this may be considered as a matter of taste or style, but there is no need to argue about that. My point is different.
The language specification doesn't say anything at all about dimensional consistency in the normative part, even with my proposed changes. Hence, a negative outcome of dimensional consistency checking should obviously result in a warning, not an error. As it does in the tools I know best.
The question is: do we prefer to get some "false positive" warnings, such as dimensional inconsistency warnings from the above-mentioned examples, but also to catch plain wrong equations such as v = sqrt(2*g) when writing new models, or do we prefer not to get those false positives, at the price of not getting any warning about plain wrong equations?
As a modeller, I have no doubts: if the price to pay to catch plain wrong equations that would lead to bogus results is to get some false positives, that can easily be dismissed by inspecting the corresponding lines of code, I'm ready to pay it.
and all of the Modelica.Electrical.Spice3.Internal.Mos formulas.
Those models are just a 1:1 transposition of Fortran code, which of course had no concept of unit. They are deliberately confined to an "Internal" package. I don't think anybody would be surprised or annoyed if you get some unit consistency warnings when checking them.
There are also some cases where people write numbers such as 0.0289651159 with an implicit unit (I assume you get it Francesco).
I get it. Let them get a warning, if they know what they're doing they can simply ignore it. If they really don't want to see those warnings, they are free to define constants with proper units and use them instead, which I personally see as a much more elegant and clear modelling style.
Based on my experience, I am a lot more worried about accidentally writing a wrong model and not being warned about it. Happened to me and my co-workers twice already in the last 3 years. The consequences are a lot worse in this case.
Clearly, we need to introduce unit checking in the specification with some sort of transition in mind.
@henrikt-ma I'm not at all sure about this. Personally, I don't think there is any need to be this strict.
In any case, we are straying away from the scope of this PR again. This PR is not aimed at introducing formal strict unit checking, not even to pave the way for it. As I said, I don't think there's any need of that. This PR has the much humbler goal of allowing to generate warnings for people that accidentally write wrong equations that are not caught by dimensional consistency checking because they contain some dimensionless numerical factor. That's it.
The fact that the specification hasn't said anything about unit errors so far means that introducing unit checking should be expected to be a backwards incompatible change. For a smooth transition into a unit-correct world, I think we should count on tools mitigating the effects of the backwards compatibility break. For example, a tool could have a non-strict unit checking mode, where unit errors are diagnosed as warnings. Exactly how to do this for an acceptable user experience should be up to the tools, in my opinion; the specification should call an error by its real name.
@henrikt-ma, AFAIK at the moment you are the only one proposing to formally introduce unit checking in modelica 😅.
From an end-user perspective, I see no need to do that, and I'm not seeing other people pushing for this. Why should we bother to introduce non-backwards compatible changes that nobody's asking for? Besides, we'd end up debating endlessly about hotly debated issues such as: is "rad" a base unit?
I'm just trying to improve non-normative dimensional consistency checking for equations containing non-dimensional factors and, yes, to slightly discourage people from mixing variables with units and numbers-that-are-supposed-to-have-some-unit in the same expression, which I find a bit lousy, by getting them some warnings when checking their models. Warnings that they can happily ignore, if they know what they're doing. No big deal.
This is even more true for Modelica.Electrical.Machines.Utilities.ParameterRecords.IM_SquirrelCageData: I find
parameter SI.Inductance Lm=3*sqrt(1 - 0.0667)/(2*pi* fsNominal) "Stator main field inductance per phase";a bit fishy.
I very much agree, that the caclculation of the default machine parameters is neither intuitive nor formally correct.
The calculation is a consequence of a machine reference impedance ZRef = 1 Ohm = VRef / IRef. This reference impedance is in turn a result of the reference phase voltage VRef = 100 V and the reference current IRef = 100 A. As neither the reference voltage VRef nor the reference current IRef are parameters declared in either machine model, ZRef could at least be used as an internal (protected) constant to calculate the inductances and resistances with consistent units.
parameter SI.Inductance Lm=3*ZRef*sqrt(1 - 0.0667)/(2*pi*
fsNominal) "Stator main field inductance per phase";
The induction machine documentation reads:
@henrikt-ma, AFAIK at the moment you are the only one proposing to formally introduce unit checking in modelica 😅.
If we are not aiming at formalizing unit checking in the specification, I don't see the point of MCP-0027. If all you care about is a tool-specific way of catching certain unit errors, just ask your favorite tool vendor to solve this in a tool specific way, but don't expect your libraries to pass unit checking in other tools, and don't expect that libraries developed with other tools will play nicely with the unit checking in your tool of choice.
No, we must keep the long term goal of Modelica code portability between tools in mind here, and our community would be right to demand of the specification that it lays down unit checking rules that make the Modelica library ecosystem reasonably free of unit errors. For this to happen, tools need to reject unit errors with support in the specification, and tools must not reject use of units that is valid according to the specification. (Where there is a gap in the specification, a tool might opt for a warning if it doesn't like of units are used.)
From an end-user perspective, I see no need to do that, and I'm not seeing other people pushing for this. Why should we bother to introduce non-backwards compatible changes that nobody's asking for?
I have always had the other impression: our users find it very strange that there's a formal syntax for how to express units, but no rules for how to check them.
Besides, we'd end up debating endlessly about hotly debated issues such as: is "rad" a base unit?
I don't deny that this is a problem, and therefore I think it is important that we don't close too many doors as we set out on this endeavor.
I'm just trying to improve non-normative dimensional consistency checking for equations containing non-dimensional factors and, yes, to slightly discourage people from mixing variables with units and numbers-that-are-supposed-to-have-some-unit in the same expression, which I find a bit lousy, by getting them some warnings when checking their models. Warnings that they can happily ignore, if they know what they're doing. No big deal.
I, on the other hand, want to make sure we don't introduce some ad-hoc rules that will not fit in the long-term development of a formal system for unit checking. I don't think it helps making them non-normative at this point, as it would be weird for tools to ignore the only rule we have about unit checking.
If we are not aiming at formalizing unit checking in the specification, I don't see the point of MCP-0027. If all you care about is a tool-specific way of catching certain unit errors, just ask your favorite tool vendor to solve this in a tool specific way
Of course I could ask the developers of omc to implement my proposal without going to the hassle of have it passed into the Modelica Specification. But that's not my intent here. I don't want this feature to be tool-specific, I want this to hold for all tools, and I want all people that write Modelica code to be aware of it and to expect it to be used by tools. I also want to make sure I'm not missing something, so I'm actually seeking the feedback and discussion of the MAP-LANG group to make the proposal as good as possible.
What I don't want to do now is to introduce a complete formal type-checking handling of units. Why? Because it won't be backwards compatible, so it will trigger endless discussions, and because it will trigger even more discussions because it's tricky (see the is-rad-a-base-unit thread). I am fine with whatever method tools implement to warn me about dimensionally inconsistent equations, I don't see any need to go further than what we have now in the specification.
Except for one tiny detail: the current specification implies that the unit of literal constants is the default unit of Real, i.e. "", which makes supposedly dimensionless factors act as dimensional wildcards. So, with the current MLS, tools will never catch dimensionally inconsistent equations as soon as they include some literal constant, no matter how smart they are. Unless they make some assumption which are unfortunately not stated in the MLS and are thus questionable, because they would be tool-specific, not Modelica-specific. So I'd like to improve that, by adding a normative rule about giving the right unit attribute to Real literals. This is not a tool-specific issue, it is rather a language feature. I want modellers to be aware of that, and tool implementors to exploit that. As with all things Modelica.
That said I'm fine with the technicalities of unit checking being left to tool implementors. I assume they can do a good job, exactly in the same way I assume they can do a good job at solving equations correctly and efficiently.
but don't expect your libraries to pass unit checking in other tools, and don't expect that libraries developed with other tools will play nicely with the unit checking in your tool of choice.
That's precisely why I want this rule about units of literals to be part of the specification. To avoid the possibility that different tools make different assumptions about it, resulting in non-portable behaviour. That's been precisely the point of MCP 0027 since its inception, some years ago 😅
I have always had the other impression: our users find it very strange that there's a formal syntax for how to express units, but no rules for how to check them.
There are also no rules on how to solve equations in the MLS, which one may find very strange, given that all tools actually do that. Should we put that in the specification as well?
I, on the other hand, want to make sure we don't introduce some ad-hoc rules that will not fit in the long-term development of a formal system for unit checking.
As far as I understand, my proposal (the normative part) doesn't do that, but maybe I'm missing something. Can you point out one or two concrete examples, where you show that my proposal would be incompatible with yours?
I don't think it helps making them non-normative at this point, as it would be weird for tools to ignore the only rule we have about unit checking.
The non-normative part in this PR is just to explain the rationale. What is important is the normative part, as usual.
I agree with this goal, and find this proposal good in itself. However, my main concern isn't about the language.
Unit checking (especially using this proposal) will not only generate the desired unit-warnings, but also unit-warnings from used libraries - like MSL (and others).
With https://github.com/modelica/ModelicaStandardLibrary/pull/4041 other unit-errors are eliminated in MSL And with https://github.com/modelica/ModelicaStandardLibrary/pull/4051 also in ModelicaTest
However, adding units of literals as proposed here will generate about 1MB of warnings for MSL, so not only https://github.com/modelica/ModelicaStandardLibrary/issues/4050 but more.
Do we have the resources to handle that? Note that some of them can be corrected in a straightforward way, but others require more - and we have already noticed that some of the changes intended to handle this have been incorrect, later corrected in https://github.com/modelica/ModelicaStandardLibrary/pull/4040/commits/cf30cb744fcc3f7faaba03793c42e0a666f2180a
The question is: do we prefer to get some "false positive" warnings, such as dimensional inconsistency warnings from the above-mentioned examples, but also to catch plain wrong equations such as v = sqrt(2*g) when writing new models, or do we prefer not to get those false positives, at the price of not getting any warning about plain wrong equations?
I'm curious why you consider it a "false positive" if this proposal uncovers inconsistencies like equating an inductance and a time, but a "plain wrong" true positive when flagging sqrt(2*g)?
Is the former not rather a case of "works as intendend", in that we uncover previously unnoticed problems in the MSL? Others seem to agree, see https://github.com/modelica/ModelicaStandardLibrary/issues/4050).
In the end I agree with your position, I just want to remark that I find it questionable to call that a "false positive" -- if anything it's a true positive, IMO.
I can't answer the resource question, but would that really be an argument to not implement better checking :interrobang: Because doing it would point out too many errors for "free"?
Could this be solved on the tool side, e.g. by implementing optional switches in the respective unit checking features? Users who just want to check their own code without being distracted by warnings from "external" code could exclude the MSL (or any other code external to their project) from unit checking.
I've seen tooling and linters from other languages do it like that. E.g. in VS Code, the debugger configuration has a flag "justMyCode": true that means the debugger will not show jumps into libraries outside the current workspace/project. This is quite convenient, because jumping around through the C++ STL functions/template code is seldomly helpful nor relevant for debugging.
Something like this seems easily achievable by pushing the unit checking results through a regex filter or somesuch.
I can't answer the resource question, but would that really be an argument to not implement better checking ⁉️ Because doing it would point out too many errors for "free"?
The problem is when your errors are "drowned" by errors by others, and the likely reaction is one of:
- Ignore/Turn off/Tune out all such errors.
- Stop using Modelica.
Could this be solved on the tool side, e.g. by implementing optional switches in the respective unit checking features? Users who just want to check their own code without being distracted by warnings from "external" code could exclude the MSL (or any other code external to their project) from unit checking.
Likely for MSL.
At first, I wasn't sure whether it was that straightforward.
The reason is that your parameters and equations interact with MSL (and other libraries) - especially if you try to deduce units (and not only check units in each individual line), and it might not be that easy to see if the problem is in your model, or in the MSL-part. With unit deduction I mean cases like https://github.com/modelica/ModelicaStandardLibrary/pull/4051 but generally more complicated.
Obviously if you look at Modelica.Mechanics.MultiBody.Examples.Systems.RobotR3.OneAxis and see J=1.3*mLoad it's clear that the 1.3 should have unit="m2" in that example, and similarly if you look at Modelica.Media.IdealGases.Common.Functions.dynamicViscosityLowPressure you see a number of unit issues inside that function. It's not always clear how to correct it - and just dividing all variables with constant Real unitX(unit="...")=1; is a bad practice that may hide errors.
However, even though all unit errors aren't that localized, it seems that all non-local unit errors in MSL are in Modelica.Electrical.Machines.BasicMachines.SynchronousMachines.SM_PermanentMagnet (and related classes with similar constructs), and the only reason they aren't localized is because there's a local pi-constant.
That suggests that one could even say that it is specific classes in MSL (and other libraries) that aren't unit-correct, which in some cases may be a better idea than focusing on patching them.
The non-local error now has a PR for fixing: https://github.com/modelica/ModelicaStandardLibrary/pull/4052
(Combined with other PRs it fixes all non-local unit errors in MSL; but there are many remaining errors; search for Incompatible units - and ignore the first one(s).) units7.zip
Thanks for that zip file, that is useful! I count/grepped 338 instances of incompatible units. It might be that there are some real false positives in that log, though.
For example, I see:
Incompatible units.
Incompatible units in sqrt(Modelica.Media.CompressibleLiquids.LinearWater_pT_Ambient.density_Unique44( valve.state_a))
The part Modelica.Media.CompressibleLiquids.LinearWater_pT_Ambient.density_Unique44( valve.state_a) has unit kg/m3
The unit issue reported appeared in the equation valve.m_flow = homotopy(valve.relativeFlowCoefficientvalve.Avsqrt( Modelica.Media.CompressibleLiquids.LinearWater_pT_Ambient.density_Unique44( valve.state_a))Modelica.Fluid.Utilities.regRoot2(valve.dp, valve.dp_turbulent, 1.0, 0.0, true, 0.0), valve.relativeFlowCoefficientvalve.m_flow_nominal* valve.dp/valve.dp_nominal); Found in class Modelica.Fluid.Valves.ValveIncompressible, C:/Users/hos/Documents/Dymola/modelica-git/Modelica/Fluid/Valves.mo at line 40, and used in component valve.
As far as I can tell, that equation is dimensionally sound, I don't know why it reports incompatible units in that sqrt (137 sqrt-related unit reports overall). Maybe a problem dealing with fractional powers?
I'm curious why you consider it a "false positive" if this proposal uncovers inconsistencies like equating an inductance and a time, but a "plain wrong" true positive when flagging
sqrt(2*g)? Is the former not rather a case of "works as intendend", in that we uncover previously unnoticed problems in the MSL? Others seem to agree, see modelica/ModelicaStandardLibrary#4050).
The difference is in the intention. In the old days (i.e., before this proposal 😅) there were no explicit rules against mixing variables with units and literal-constants-that-are-meant-to-have-some-unit-but-do-not. So, those models could actually turn out to be perfectly fine. Maybe a bit ugly, but that was more an aesthetic judgement. The model was "correct" in the sense that produced the intended results. From this point of view, modelica/ModelicaStandardLibrary#4050) is not wrong, because it computes the right results. It is only a bit hard to read a posteriori.
In my case, I wrote a pipe model for the MEV project and I accidentally forgot to multiply the friction term of the momentum balance equation by the length l of the finite volume of the pipe. This would have been caught immediately by dimensional consistency checking when I checked the model, were it not for a leading Cf/2 term, where Cf is defined as a dimensionless Fanning friction factor, and 2 is, alas, a Real literal constant, which is meant to be non-dimensional, but was recognized as a dimensional wildcard by the tool, preventing to catch the wrong equation. Luckily, the numerical value of l was close to one (meter), so when I finally spotted the error and corrected it, the simulation results didn't change significantly, but that is another story. I was just lucky, could have been much worse.
So, with the old rules, whether the dimensional inconsistency warning is a false positive or a true positive requires human judgement, possibly by the original author who is the only one that knows what his intentions were, see the modelica/ModelicaStandardLibrary#4050 case.
This is actually why I think it's good to go ahead with this proposal, because one shouldn't have to guess the author's intentions regarding literal-constants-that-are-supposed-to-have-a-unit, but should rather make that explicit, as the modelica/ModelicaStandardLibrary#4050 case demonstrates.
I can't answer the resource question, but would that really be an argument to not implement better checking ⁉️ Because doing it would point out too many errors for "free"?
The problem is when your errors are "drowned" by errors by others, and the likely reaction is one of:
* Ignore/Turn off/Tune out all such errors. * Stop using Modelica.
I agree that this is a potential issue and we have to take it into account. However, I don't see it as such a big problem in our case.
As I understand, you get al lot of warnings if you check the entire MSL, which contains thousands of models. We may be worried that implementing this proposal exposes some (uncritical) issue with MSL, if one wants to check it because he doesn't trust it, but I don't think regular users do that. They will check the models that they are writing, and it is important that true errors are not drowned in false positives in that case.
This doesn't seem to me to be the case. Most errors in the report you sent out are either in the Spice library (which is probably not very much used, and could contain an explicit warning about this issue in the documentation), or in examples, such as the case of modelica/ModelicaStandardLibrary#4050. So, it is quite unlikely that they will be involved when a user checks his own models. As a measure of quality improvement, we could also try to improve the Examples in the MSL from this point of view, also to show newbies how to do things properly, but this is not critical IMHO, as long as people ususally don't spend their time checking the MSL examples to find issues 😅
The remaining errors should be investigated, and I don't think they are that many. I can take care of that myself. As to the valve model mentioned above, I wrote it myself and the intention was definitely to have it dimensionally consistent, I'm not sure why it is reported as dimensionally challenged. Maybe it's a tool issue?
Thanks for that zip file, that is useful! I count/grepped 338 instances of incompatible units. It might be that there are some real false positives in that log, though.
For example, I see:
Incompatible units. Incompatible units in sqrt(Modelica.Media.CompressibleLiquids.LinearWater_pT_Ambient.density_Unique44( valve.state_a)) The part Modelica.Media.CompressibleLiquids.LinearWater_pT_Ambient.density_Unique44( valve.state_a) has unit kg/m3
As I understand, this is not a false positive, but a tool issue.
The textbook valve equation is w = Av*sqrt(rho*dp) which is dimensionally consistent (it is derived from Bernoulli's law) as long as the proper SI unit is used for the Av coefficient, i.e. square meters, which is actually the case. This coefficient is never used in practice because you get very small numbers, so people use Kv (metric, but using engineering units) or Cv (US units with gallons and psi), but that is handled appropriately in the model. BTW, figuring out the conversion factors from Cv to Av is quite a challenge, but the well-known result is very simple Av = Cv*24e-6. It also turns out Cv and Av have similar values, since Av = Kv*27.7e-6, and unfortunately Cv is still very much used on this side of the Atlantic, as the gauge of pipes in inches. The 27.7 factor with decimals comes from the square root of 1e5 when converting bar to Pa 😅
The problem here is that you can't use sqrt() when you get close to dp = 0, so you use Utilities.regRoot2() instead. This function is a regular Modelica function defined in the Media library (not a built-in one like sqrt()) which takes a generic input with any unit, so there is no way to express in Modelica the fact that the output unit should be the square root of the input unit. Hence, this function is just defined with empty unit attributes, both for inputs and for outputs, and this makes it impossible to do any dimensional consistency checks in this case. Amen, case closed.
Therefore, as I understand, according to the proposal of this PR this equation should not be reported as dimensionally inconsistent.
As I understand, this is not a false positive, but a tool issue.
Sorry if this was not clear enough. This is what I meant, a false detection by the tool.
Thanks for that zip file, that is useful! I count/grepped 338 instances of incompatible units. It might be that there are some real false positives in that log, though.
As far as I can tell, that equation is dimensionally sound, I don't know why it reports incompatible units in that
sqrt(137 sqrt-related unit reports overall). Maybe a problem dealing with fractional powers?
Well, fractional powers aren't really specified - and you cannot specify them for variables.
However, there's more serious mistake in that log-file. On the other hand I think that mistake hints at a better proposal, I will check what happens.
There are also no rules on how to solve equations in the MLS, which one may find very strange, given that all tools actually do that. Should we put that in the specification as well?
I find the comparison to equation solving misleading. Unit checking is not like equation solving, it's like static type checking. No type system makes everyone satisfied, but still many of us prefer to use programming languages with static type checking. One may sometimes need to express things in certain ways to please the type system, but it's a price one is willing to pay for the overall safety guarantees that come with the use of a static type system. We already have static typing in Modelica, so I expect our user base to also appreciate and be willing to pay the price for similar guarantees when it comes to unit handling.
For example, I think many users would prefer having a decision on "1" vs "rad" instead of undefined behavior left to the creativity of tools. Some will get exactly what they want, others will have to either accept that there are certain common errors that wont be caught, or accept that one has to make some manual conversions to comply with unit checking rules.
The updated idea is that we shouldn't consider all numbers appearing in expressions with other operands.
Instead consider specific cases and say that it has unit="1" when the other operand have a non-empty unit in:
- Multiplication and division
- Also in addition and subtraction
- Also in relational expressions. These are numbered and the intent is that they are different levels.
I checked level 1 above, and I believe we need to stay at that level to avoid excessive warnings.
Also considering addition and subtraction seemed to only give false positives(*), and relational expressions just gave lots and lots of false positives. Requiring users to change r+1e-12 or abs(r)>1e-12 to 1e-12*unitLength doesn't seem helpful.
I can understand that they in some cases will detect errors, but mostly it will just be false positives that will cause problems.
*: Except that it also detected an issue in Modelica.Magnetic.FluxTubes.Shapes.HysteresisAndMagnets.GenericHystTellinenHard I don't see why it wasn't detected before.
However, it is an interesting case for what happens when your try fix unit-correctness without thinking it through and/or are tricked by the variable names.
All other H's are MagneticFieldStrengths and the following are the relevant equations:
parameter Real M = 10/Hc "Slope of tanh()-function
final parameter SI.MagneticFieldStrength H0= 0.5*log((1+mu0*Hc/Br)/(1-mu0*Hc/Br)) + M*Hc;
equation
tanhR = tanh((M*H - H0)/unitH);
tanhF = tanh((M*H + H0)/unitH);
Clearly that is wrong for units.
My guess is that this is a bad case of unit-casting, and they should be totally removed:
parameter Real M = 10/Hc "Slope of tanh()-function
final parameter Real H0= 0.5*log((1+mu0*Hc/Br)/(1-mu0*Hc/Br)) + M*Hc;
equation
tanhR = tanh(M*H - H0);
tanhF = tanh(M*H + H0);
Where M.unit="m/A" and H0.unit="1" (could specify H0 if you want).
This highlights that any equation containing a unitX is suspicious.
For Tellinen there's now a correction PR: https://github.com/modelica/ModelicaStandardLibrary/pull/4053
I believe that it may be possible to reformulate this, and also making it more similar between different operands.
The key insight is that instead of thinking that 0.7 in 0.7*x or 0.7+x should have the unit "1", we should view it as 0.7*x and 0.7+x having the same unit as x. For multiplication and division, the consequence is that the literal has unit "1".
I agree that writing 0.7+x isn't ideal; it's just that flagging it as a unit-error gives more problems than it solves.
Additionally, we should avoid "fixing" such cases by adding unitX and writing e.g., 0.7*unitV and instead ideally avoid the literal in the equation and write vThreshold+x where constant SI.Voltage vThreshold=0.7;
Well, fractional powers aren't really specified - and you cannot specify them for variables.
Thanks, interesting, I was not consciously aware. Still, physically/mathematically it's ok here (simpler example: sqrt(vel1)*sqrt(vel2) gives a velocity).
Is this detection an artifact of the expression tree containing a node with fractional powers (the result of that sqrt) being unexpected? Maybe something that could be amended?
Multiplication and division
Also in addition and subtraction
Also in relational expressions. These are numbered and the intent is that they are different levels.
Do we also need to include the exponentiation operation in that list?
Also considering addition and subtraction seemed to only give false positives(*), and relational expressions just gave lots and lots of false positives. Requiring users to change
r+1e-12orabs(r)>1e-12to1e-12*unitLengthdoesn't seem helpful.
I understand your concern that some flagged things are considered inconvenient as they generate a lot of reports. However, I don't think it's helpful for tackling the unit checking issue to call those "false positives", because they are not---they are showing a real dimension issue!
Instead of requiring users to use 1e-12*unitLength, it would probably be better practice to do what you yourself proposed further below, i.e. use constant SI.Length l_threshold = 1e-12 and abs(r)>l_threshold.
Additionally, we should avoid "fixing" such cases by adding unitX and writing e.g., 0.7*unitV and instead ideally avoid the literal in the equation and write vThreshold+x where constant SI.Voltage vThreshold=0.7;
:+1:, unitX seems like a code smell to me. (incidentally also used in your PR https://github.com/modelica/ModelicaStandardLibrary/pull/4052 ;-) )
The updated idea is that we shouldn't consider all numbers appearing in expressions with other operands.
Instead consider specific cases and say that it has unit="1" when the other operand have a non-empty unit in:
1. Multiplication and division 2. Also in addition and subtraction 3. Also in relational expressions. These are numbered and the intent is that they are different levels.
I have (manually) checked this proposed literals behaviour against the original list of unit tests (no pun intended :D) by Francesco, and this largely seems to be compatible. However, one example shows different behaviour:
specificEnthalpy_pT(1e5 + time*1e5)is rejected
This would not be allowed because of rule #2 above, which would allow an addition of a time (result of time*literal and rule #1) and a unitless literal. I agree with the original test behaviour that this should be rejected. This makes me question if we should adapt #2 here?
(Depending on how much the proposal is updated, we need to make sure that function args and assignment/equality behaviour is covered, too)
However, one example shows different behaviour:
specificEnthalpy_pT(1e5 + time*1e5)is rejected
That should still be rejected:
timehas unit "s"time*1e5thus has unit "s",1e5+(time*1e5)thus has unit "s",- I assume the call should be
specificEnthalpy_pT(1e5 + time*1e5, 293.15); and that is rejected since the input has unit "s" not "Pa".