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
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
Changes from 2 commits
7524b11
365663f
0dc006c
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 understand that this was designed to handle
der(x)=-x;
without reporting errors.However, as far as I understand that also means that if we have
der(x)=velocity;
we cannot deduce the unit ofx
, which would be a step backwards. To me it instead makes sense to state that only variables that occur exactly once can be solved from the constraint - which gives a similar result for thisder(x)=-x;
, but handlesder(x)=velocity;
.The disadvantage is that we can then not deduce the unit of x in
x*x=area;
orx+x=area;
. However, such examples are bit silly, especially as they can be written asx^2=area;
or2*x=area;
.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
velocity
is a wellformed unit, we can simplifiesder
in the constraint andD
becomes empty. So we can in fact solve forx
.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.
At least this indicates that the rules are not very clear; if we have to sometimes simplify things in constraints etc.
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.
Feel free to rephrase so that it is clearer.
At the end of the day the complexity associated with relaxing unit-checking is why we felt the need to spell out the algorithm in painstaking details in this document.
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.
My proposal is not rephrase it, but instead state that only variables that only occur once in a constraint can have their unit 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.
No, since we cannot start the process.
Similarly as not being able to deduce units for
x
andy
based on:sin(x/y)=...; cos(y*x)=...;
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.
In the language of this proposal, you would want for
sin
andcos
to not impose any constraint on their argument, as well as expressions containingder
to haveundefined
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.
I would rather that:
sin
andcos
impose unit="1" (or "rad") for input.sin
andcos
impose unit="1" for output.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.
Does that mean that this model is valid according to your 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.
Yes, and x.unit="m" (based on c=x... where c.unit="m") and y.unit="1" (based on
c=...+x*y
; where c.unit="m" and x.unit="m"). I agree that it takes some care to specify the constraints that unit(a+b)=unit(a)=unit(b); to not get stuck on unit forx*y
.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
sign
function is as far as I understand not a transcendental function, as they are defined as non-polynomial analytic functions, and I'm a bit unsure aboutatan2
.More importantly neither of those functions should use the rules for transcendental functions:
atan2(y, x)=atan(y/x);
i.e. the two inputs should have the same unit - but it is perfectly normal to use atan2 for variables with 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.
It does if it is a temperature.
This is what is specified here.
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.
No.
Or with more words: it's more complicated - the unit sort of matters if it is an absolute temperature, but not if it is a temperature difference (and the unit system doesn't separate the two). And in the SI-system, which we base the unit handling on, absolute temperatures are measures in Kelvin and using the sign for an absolute temperature in Kelvin is kind of meaningless.
Similarly you should normally use sign for voltages, not potentials; and for relative positions (or velocities) - not absolute positions. The unit system doesn't separate them, and there are lots of models using sign for variables with units (fluid, mechanical, etc) - and they make sense.
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.
Fair enough for
sign
, I agree that we would need to ensures that the value is a difference.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 just had another look at sign, the specification doesn't enforce any unit on its argument, it is just specifying that
sign(-1)
should behave as a literal.