Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Handle units of literal constants by means of rules for the empty unit #3257
base: MCP/0027
Are you sure you want to change the base?
Handle units of literal constants by means of rules for the empty unit #3257
Changes from 7 commits
7524b11
1309d10
2dfb4e8
0c62f41
203c16e
5d42065
fc4abde
b86567b
1f5131c
3291a73
f639afc
3a3b1f0
e411347
f3755a2
fccda32
89092ba
82bb062
99ff3c9
13529f7
847e645
ebfc229
232c4fa
9aeb15f
a28b795
089f554
35e4f01
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not a friend of "soft" qualifiers like "certain", "some of", etc. in specification texts. I think it would be better to enumerate all the specific places/sections/cases you mean to include in this statement. Include a "fall-through" case handling the situation if no element of the enumeration fits, if appropriate.
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.
The paragraph is part of the section introduction, so it isn't meant to convey the details. I agree that later, when the actual rule is given, one has to avoid formulations that risk causing unnecessary ambiguity.
Does it work better now, with the reformulated presentation of the corresponding list of cases? (I don't think this list needs a fall-through case, as it should always be possible to add more cases in a backwards compatible way.)
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.
Should it then be marked non-normative? Otherwise, it's normative, right? (I'm not too firm on the procedural details of this)
I can't tell which list you mean here?
This is not a contradiction! You can first specify (random example: operators covered by some procedure):
and then later refine by adding more cases
The set of the fallthrough/uncovered cases shrinks by adding more cases, but it's always crystal clear which ones are "undefined" (first: everything except +,-, then everything except +,-,*,/)
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'm not sure. It's a matter of style where we have at least decided to not explicitly mark chapter introductions as non-normative even though their nature is non-normative. In the case at hand, I'd say that the (see below) should be sufficient to ensure the reader that there's a specific meaning of certain places to be found by looking at the sections below.
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 refer to formulations like this one:
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.
The nice thing about this list (When encountering the empty unit in…) is that it doesn't leave any situation undefined. In the other cases, the unit isn't determined by the context, but always becomes
"1"
as (now) described directly below the list.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.
Comparing it to #3266 (comment) it seems that it has the same critical flaw.
In general, my issue with some proposals is that there are way too much text and theory, but too little actual practice.
I can see that the other proposal - changed to not apply to addition and subtraction - does detect actual modeling issues; but also that it finds a number of issues that aren't that important and will take time to correct. I don't know if people are actually willing to put in the effort to correct - and I don't know the corresponding result for this proposal.
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 think it needs to be seen in the light of me looking for a solid ground on which to implement unit checking in System Modeler, where the specification at least marks a starting point for something that can evolve into a set of rules that make unit consistency a model feature that is portable between tools.
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.
Can you explain what you mean by that, because to me this proposal is very clear?
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.
You are just describing the properties of a sound logical system, as opposed to a complete one.
At the end of the day, if MA wants the models produced with modelica to be portable it will have to opine on where unit checking should fall on the sound to complete spectrum. And most people, expects a sounder system when we talk about type checking. This will obviously imply a lack of backward compatibility, but then again the spec does not currently support the rejection of models relying on invalid unit use anyway.
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.
Actually, in these cases you don't 'force inference of unit "1"', nor propagate the empty unit, but these expressions (i.e. the operators + operands) propagate the unit of the operand with the non-empty unit (addition, subtraction, multiplication, division when in numerator) or its reciprocal (division when in denominator).
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.
Maybe better?
"If an addition, subtraction, multiplication and division, operands that have empty unit get inferred unit "1" if the other operand has non-empty unit, and remain empty-unit otherwise"
..and deal elsewhere with the correct units for these operators' result when both operands have been assigned units (both empty, one empty, both non-empty cases)
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.
The way it is formulated comes from the need to leave gray areas open for future design, with the understanding that tools may take the easy way out and propagate empty units up the expression three whenever the specification doesn't define what to do, in order to not reject things without support in the specification. I am thus categorizing all expressions (where one or more subexpressions have empty unit) according to:
Maybe there are alternative ways of achieving the same effect, but I'm not sure how to do it without ending up with a proposal that covers more than what the language group wants as a solution for MCP-0027. In the future, when all cases are covered by the specification, there should be an easier way to express the rule:
"1"
is inferred.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.
To be clear, it's fine to have grey areas (if you mean areas that the spec does not cover), they just should be clearly delineated. IMO, having grey areas is not a contradiction to having a clearly defined process for the rest.
I think I understand what you mean better, but I'll have to think on this a bit, I'm a bit confused.
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.
This that mean we are ready to close this conversation as resolved?
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.
If you want to specify a "bottom-up" approach as you have previously expressed, an expression cannot be required to know about its parents, only children.
I suggest reformulating the operator items throughout to be from the perspective of the operator expression, not the operand, and adjusting the resulting units of other expressions accordingly
E.g. here 'For binary exponentiation, if the right operand has empty unit, infer unit "1" for it'
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.
That's a good point that I've also considered myself. The bottom-up nature of the approach needs to be crystal clear, or else much of the point of this proposal is lost. I'll start with something that may be overly clear, and then one might be able to shorten the formulation once everybody is on board…
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.
Better now? (It turned out that I also needed to say a few words on unit propagation in order to separate between the declared
unit
-attribute of a variable, and its unit after unit propagation; for most purposes, it is the unit after propagation that is of interest.)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 like what you added in the section
bottom-up-unit-derivation
(although the flow could be further simplified I think). Does this really change the fact that here an expression (the operand) needs to know its context (I'm the right operand of a binary exponentiation) to decide how to act, which violates the bottom-up principle?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'd be happy to hear your thoughts about how to further simplify the design or its presentation. (I've considered completely separating everything concerning the empty unit from the reasoning about non-empty units. Then I think one would end up with a fairly short list of rules for where empty units may appear in the expression tree, and how they can be propagated up the tree.)
Yes, this is the idea I'm trying to convey. Consider determining the unit of exponentiation here:
x
and(1 + 1)
by themselves. Here,x
has unit"m"
, and(1 + 1)
is anInteger
expression implicitly cast toReal
, so it has empty unit.x
and(1 + 1)
are known, it is time to consider the context of being operands to exponentiation. Here, the particular rules for the operator at hand comes into the picture. In particular, one needs to see if there is a rule giving meaning to operands with empty units; when that's the case, no unit inference takes place. Otherwise, unit inference takes place, implicitly casting the empty unit to a non-empty unit.x ^ unit((1 + 1), "1")
, whereunit
is the pseudo-code operator taking an operand with empty unit and producing the corresponding expression with some other unit attached.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.
Also addition/subtraction? Or all operators? I'm not sure...
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.
Yes, this is where I see a possibility to simplify the presentation by concentrating on just the propagations of the empty unit up the expression tree. Then we get this simple rule:
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.
Sounds good.
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 gave it a try, and it seems to me that this led to considerable simplification of the proposal. Maybe we could consider this conversation resolved, and continue the discussion in conversations attached to the updated text?
(By the way, I'll be away until Monday.)