-
Notifications
You must be signed in to change notification settings - Fork 40
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
Applying Hindley-Milner to unit-checking #3491
base: MCP/0027
Are you sure you want to change the base?
Applying Hindley-Milner to unit-checking #3491
Conversation
c0fe5d3
to
365663f
Compare
Co-authored-by: Henrik Tidefelt <[email protected]>
I will have to study it in more detail - and in particular would want to first consider just the definition of the unit-checking and then consider the variants for adding units. For the unit-checking I noticed that several functions were not considered (but could trivially be added - Dymola already supported these checks except
And something for min, max, transpose, diagonal, sum, product. Dymola also treats the different transcendental functions differently regarding "rad" vs. "1"; even though those differences are later ignored. |
It was a conscious choice not to include an exhaustive list of builtin functions as, as you said, they can be trivially added later if we agree on the base mechanism. Similarly, for arrays, previous attempt at standardizing unit-checking have focus on scalar expressions, but as mentioned in the document it can be generalized to element-wise operations on arrays. And we are confident that it will work with matrix operations too, but we don't have a proof of concept for that last part. |
Three comments on the proposal.
In general, I think this is the kind of extension that is best formalized through a layered standard, as there is not change for correct models. |
As far as I understand there are three parts to this (now that we added fractional powers in the unit-grammar):
Regarding (1): Of which unit-constraints are active. There are some minor details of which unit-constraints are active that are important (only active equations?, and clearly allowing different units for different instances of the class corresponding to "let-polymorphism"). Note that these details are important if we are considering storing the deduced units in the model (only possible for the current model, and even then it can be problematic). Having some way of de-activating unit-checking per equation (or model, package) also seems needed in practice, but is just a minor detail. Regarding (2): The handling of literals is the major new change; and originally Dymola's unit-check had literals as total wild-cards which wasn't good triggering this MCP, whereas requiring units for every literal would be too much work for users for too small a benefit. Regarding (3): One alternative for solving for units one variable at a time is to view the unit-constraints as a large (over and under-constrained) system of equations in rational numbers; another is to have more ad-hoc rules (so that we have one set of rules for checking units and another for reducing - we would then need to ensure consistency etc). Having some restriction on when to deduce (like for (global) constants can still be added to the current idea. To illustrate the alternative of system of equations consider two problematic equations:
Assuming no units are given the approach with solving for variable-units one at a time cannot find any units. With unit-constraints as equations we would for the first equation have: And for the second equation: Obviously this alternative would be more powerful, but:
Regarding diagnostics when it fails I would more say it is a matter of finding the possible problematic equations, and in my experience analyzing that even for a handful of equations takes care - and domain knowledge; whereas this other method seem designed for a large number of equations. |
For the power operator I would just split it based on the type of the exponent:
|
What would be the unit of
For context in the current state of the proposal, we would consider both models invalid because |
@qlambert-pro I would consider your example an ill-conditioned model without physical meaning and rewrite it:
|
I can see this - and it would be similar to figuring out the unit for |
This all makes probably little sense physically, but for cybernetic model parts I wonder how to easily lookup a magnitude with implicit |
@gwr69 I'm an engineer, to me it's not cumbersome. I prepare my equations from textbook or from my own research before I start modeling. As long as equations are not meeting in units they're bad and not fit for modeling. In some cases - even from a physical point of view - you have to use dimensionless variables, i.e x/x_ref. In many cases x_ref has a meaningful value different from1. |
@AHaumer I always envy the engineers and how they use 7 base units for everything. Coming from economics and business modeling, I would certainly like to apply the rigorous procedures and unit checking that you describe—and to which I whole heartedly agree—to more abstract fields than engineering. Unfortunately, there are no monetary units in Modelica and units of time typically end with the "day" ignoring that even in scientific realms years are frequently used, even though they are typically variable units of time. Of course, modeling in those abstract and "soft" realms will appear simple, if you only allow yourself the use of |
This proposal helpfully provides an operator just for that purpose: |
Which this proposal treat a similar way, it requires both side of an if whose condition hasn't been evaluated to have the same unit. |
I find it sub-optimal if a particular algorithm ("Hindley Midler") is required in the specification: To my knowledge, a concrete algorithm was never required in the Modelica Specification, simply because there might be different algorithms with pros and cons and the tool vendor should decide which algorithm to use. Please, add examples to the proposed specification, as already mentioned. If better unit-checking is introduced, it must be introduced in a way, that existing models can still be compiled and simulated, otherwise, people will be very upset. From a tool perspective, there should be an option to either ignore unit checking, give a warning or an error (giving a warning as a default). From my experience with Julia this is really essential, because very strict unit checking (as in Julia) can be a complete nightmare for a modeler who just wants to quickly produce a result and does not have time to search for a unit issue. The question is, how to define this in the specification, in order that unit checking is optional. |
I think the answer is simple from your analysis: Use only the units for one variable at a time, because simpler and better diagnostics. If the user is not satisfied with unit propagation, he just has to add more units at some places. Its much more critical if it is hard to figure out why a tool has selected a unit based on the analysis of many equations |
Making the Hindley-Milner algorithm a requirement of the specification was never our intention, but it relates to a state of the art type-system, which shares a lot of properties with the way we want unit to work in Modelica. We felt the need to spell it out in this document for two reasons:
|
But consider the following alternative (not efficient):
Will this give the same result? (Except for error diagnostics when it fails.) |
There is no such thing as unit-checking in the specs. This is literally what this MCP is about. |
One could also write: |
I think the place it may differ is for models like HelloWorld. Where, a sensible unit-system would find an error, but the Modelica community wants it to be valid. Which we have found to add complexity, and justified us spelling the algorithm out. |
I don't see how it would differ (I assume HelloWorld means a simple |
Great example! Make it What took getting used to for me (the BusinessSimulation as of |
So I don't think they would necessarily differ from Hindley-Milner. Although, the details contained in "If we can uniquely deduce the unit for one variable" can mean that a tool is able to infer something that another isn't but there is an inconsistency in the way a variable is used.
on the basis that if a user doesn't set units for their variables they probably aren't interested in getting unit diagnostics. It was my understanding that people like Martin would probably would like that to be the case. But if I misunderstood, I am more than happy to consider that an error. It will simplify the proposal a lot. |
I think that this is a tool issue, on our side we are both thinking of ways to make the derivation more intuitive as well as showing the user how we reached our conclusions. |
BTW: The main change in Dymola's handling is that instead of creating a substitution list each constraint is (potentially) handled individually. Added: A previously discussed consequence of this is that there is no need to treat the der-operator specially. |
And that it is done before expansion (this has both benefits and disadvantages). Clearly unit-checking shouldn't in itself cause expansion as that will be too confusing. And ThermoFluidstream demonstrates that unit-checking after expansion of evaluable parameter allows a style where the unit depends on an evaluable parameter (whether people like that is another matter); the primary downside is that it adds complexity to the overall processing - including expansion. |
So in terms of this PR Dymola's handling can be described by replacing "Solvability of constraints", "substitution" and "the algorithm" by something like:
Obviously it will infer units in fewer cases than the Hindley-Milner based one - the benefits are that it gives easily understandable derivation of units, avoids special cases for |
What is solved also depends on the order in which the constraints are processed, as I pointed out above. This approach would at least require defining an order of processing if we want to agree on which models are valid. I also don't understand why it avoids |
I cannot see that. Obviously the order will matter if there is an inconsistency, but I don't see how it can matter in other cases.
There are no simplifications of the constraints! |
As a simple example consider Modelica.Electrical.Machines.Examples.ControlledDCDrives.Utilities.DcdcInverter corrected in modelica/ModelicaStandardLibrary#4041 and how we can infer the unit of
|
BTW: Note that MCP/0027 is now rebased on master and this PR is intended to be merged into MCP/0027. |
I think simplification is necessary in order to be able to reason about units. For example, I assume one would like the same unit inference in the following equations, and I don't see how that would happen without some sort of intelligent processing of the constraints:
|
Even without any "back-substitution" unit derivations can be long and more or less impossible to see through without tool support. An implementation which strives to keep down the number of back-substitutions (not difficult) will then end up with the same sort of derivations in 99.99% of the cases, a somewhat more intricate derivation in the remaining 0.01% of cases. Solving equations is at the core of what a Modelica tool does, and the equations we encounter in the form of unit constraints are trivial compared to all the structural and numerical challenges we fight in normal Modelica equations every day. Tying the hands of tools so that they are not allowed to process these equations just because there are mutual dependencies seems exactly like the kind of thing the specification shouldn't interfere with. (It is bad enough that one wouldn't be allowed to solve all constraints due to presence of "non-expanding |
I'm neither sure about that assumption, nor whether such equations (especially with unknown unit for |
Here is the data for us, I collected the issues and PRs you mentioned, as well as the ones we reported in the last year or so, and assessed whether we were detected them with our implementation of our proposal. I have also attached the errors we report when running all examples in the three libraries/versions. They do not include the errors in the MSL when testing ThermofluidStream, neither do they include the errors in ModelicaTest.
modelica-4.0.0.txt |
The messages are quite terse. I could recognize many of them, I find at least the following confusing:
It is from
we see that
The "Ampere" just seem confusing for a model without any currents. The model improvement would be to use
|
We haven't really tried for arrays yet. The errors do come from validating the examples, I realise now that the messages my lack that context. If it makes it easier I could give you the example that was being validated. |
We need to focus on the reject/accept (detect/miss) aspect, and primarily see messages as a quality-of-implementation thing. I say primarily, because I realize that a good proposal for unit checking must not make it impossible to generate good messages. Regarding the WSM implementation, I feel confident that we'll be able to generate more intuitive messages with some additional effort, but I'd like to avoid this delaying the standardization process. |
Yes in general, but it limits understanding for the discussion and the 2nd example showed that it gave a message for a model in isolation, even though the problem only occurs when using that model in other models. That can be:
However, the bigger message I see is that we need a way to escape the unit-checking in specific classes/equations based on how some functions are written. |
Looking back I realize that we still don't know the following regarding the Hindley-Milner part:
All of this matters. Specifying a new algorithm for finding units with unclear diagnostics that only help in contrived examples does not seem attractive. |
Regarding arrays we have tried the following in Dymola (as far as I recall - will have to double-check):
As far as I can see this can deduce correct units for lots of models in MSL, and without any false warnings. Merely assuming that all arrays have homogenous unit isn't good, and will fail for some cases (in particular the block that output {abs,arg} as a Real vector for complex numbers; and Media-functions of course). I'm aware of the following issues:
However, it seems like a pragmatic solution. |
|
||
Unit checking is the process of inferring the units of Modelica variables, and ensuring the variables are used consistently in expressions and equations. | ||
|
||
It takes place after any evaluation of evaluable parameters and no evaluable parameters shall be evaluated during this process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify - it is important:
- ThermoFluidStream has units depending on evaluable parameters (different if-branches with evaluable condition)
- Exponents
Alternative implementation: - collect unit-constraints from original equations based on evaluated parameters; should give same result.
@qlambert-pro has variant text for this
(This must be true as long as tools allow the users to opt-out from unit-checking.) | ||
|
||
It is performed by using a type inference algorithm, albeit adapted to work with units. | ||
The algorithm proposed here is based on the Hindley-Milner algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not include this detail in the specification.
And as first step only handle scalars.
|
||
The unit of a Modelica variable is the unit associated with the variable after unit-checking has been performed, possibly remaining unknown. | ||
|
||
Note that the unit of a variable is not the same concept as the `unit`-attribute of the variable, but in a unit-consistent model, the unit will be equal to the `unit`-attribute when present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hans: Prefer something like.
If the variable has a unit-attribute that will be unit of the variable.
Otherwise it may be derived.
This section outlines the different constructs that comprise meta-expressions. | ||
|
||
### Variables | ||
The unit of the Modelica variable `var` is represented by the unit variable `var.unit`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative u[var] or unit[var] ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, there will be better options for how to typeset once we move to LaTeX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but the goal was also to decouple the unit of a variable from its unit-attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we should find a notation without such coupling.
|
||
After meta-expression evaluation and gathering of variables in the single-sided form, a constraint's solvability can be determined using: | ||
* A set `V` of unit variables present with non-zero exponent. | ||
* A set `D` of unit variables, present inside a `der` meta-expression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to first describe opt-in (for der(x)=x; error) and then this add-on.
* Otherwise, `e.unit` is `var.unit`. | ||
|
||
### Literals | ||
`Real` literals have `empty` unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integers are handled by cast to Real (so that no unit-checking for integer expressions).
`e1.unit` must be equivalent to `e2.unit` | ||
|
||
#### Power operator | ||
* `e.unit` must be equivalent to `e1.unit ^ e2`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include sqrt and nthPower for rational ones, and don't do it for power operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the need to cover sqrt
and nthPower
, but why not have rules for the power operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, separate the power operator into two:
- The one with integer exponent as above
- Non-integer exponents where both e1 and e2 must have unit "1"
|
||
The natural generalization to functions with multiple outputs applies. | ||
|
||
### Transcendental functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this will give different rules for the inputs to sin and Modelica.Math.sin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the difference that sin(angle)
cannot be used to infer that angle
has unit "rad"
, whereas this can be inferred from Modelica.Math.sin(angle)
? We are aware of this difference, but perhaps it needs to be pointed out in some example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not improve it so that the rules are the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I imagine that we want the built-in functions to be usable in unit-free expressions like this (it is not strictly necessary to think of sin
as a function for trigonometry):
Real x(unit = "m") = 1.0 + sin(1.0);
At the same time, I think we would like this to be a unit error as long as the input of Modelica.Math.sin
has a declared unit:
Real x(unit = "m") = 1.0 + Modelica.Math.sin(1.0);
However, I think it is too early to think deeply about how unit checking for the non-builtin functions should behave.
If [`cond` was evaluated], `e.unit` must be equivalent to the unit of the selected branch. | ||
|
||
Note that this requires introducing a intermediate variable to represent `e.unit`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add other functions/operators like: sample, hold, fill, etc.
This is fairly complete proposal for unit-checking based on the Hindley-Milner inference algorithm. This corresponds to the implementation in Wolfram System Modeler.