Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7524b11
Initial commit of ReadMe.md for MCP-0027
henrikt-ma Oct 4, 2022
a762fc7
Added "four-line" proposal for units of literals
casella Oct 25, 2022
bdbea3b
Reformatted to use sentence-based line breaks. Added inline codes for…
casella Oct 26, 2022
eb116d7
Wrote out LHS and RHS
casella Oct 26, 2022
b676ad9
Outputs of transcendental functions should be dimensionless
casella Oct 26, 2022
11dc52d
Fixed type
casella Oct 27, 2022
9a1deda
Updated proposal
casella Oct 28, 2022
df6242b
Update chapters/lexicalstructure.tex
casella Nov 2, 2022
5d5f466
Update chapters/lexicalstructure.tex
casella Nov 2, 2022
398c013
Update chapters/lexicalstructure.tex
casella Nov 9, 2022
7a9f340
Update chapters/lexicalstructure.tex
casella Nov 9, 2022
77eaae7
Update chapters/lexicalstructure.tex
casella Nov 9, 2022
8916f6e
Update chapters/lexicalstructure.tex
casella Nov 9, 2022
2c817dd
Update chapters/lexicalstructure.tex
casella Nov 9, 2022
942776f
Update chapters/lexicalstructure.tex
casella Nov 9, 2022
b3e0dc1
Update chapters/lexicalstructure.tex
casella Nov 9, 2022
3370313
Update lexicalstructure.tex
casella Nov 10, 2022
7539f1b
Update lexicalstructure.tex
casella Nov 10, 2022
15bb554
Update chapters/lexicalstructure.tex
HansOlsson Nov 11, 2022
26bce83
Update chapters/lexicalstructure.tex
HansOlsson Nov 11, 2022
ba23447
Update chapters/lexicalstructure.tex
HansOlsson Nov 11, 2022
4a7c941
Update chapters/lexicalstructure.tex
HansOlsson Nov 11, 2022
0f5c46a
Update lexicalstructure.tex
casella Nov 15, 2022
e325f6f
Used LaTeX instead of rST for itemized list
casella Nov 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions RationaleMCP/0027/ReadMe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Modelica Change Proposal MCP-0027<br/>Units of Literal Constants
Francesco Casella, Henrik Tidefelt

**(In Development)**

## Summary
The purpose of this MCP is to allow more unit errors to be detected by giving more expressions the unit `"1"` instead of having an undefined unit.
The problem with undefined unit is that it gets in the way of carrying out checking of units (which tools tend to deal with by not checking units at all where this happens).

## Revisions
| Date | Description |
| --- | --- |
| 2022-10-04 | Henrik Tidefelt. Filling this document with initial content. |

## Contributor License Agreement
All authors of this MCP or their organizations have signed the "Modelica Contributor License Agreement".

## Rationale
FIXME

## Backwards Compatibility
As current Modelica doesn't clearly reject some models with non-sensical combination of units, this MCP will break backwards compatibility by turning at least some of these invalid.

## Tool Implementation
None, so far.

### Experience with Prototype
N/A

## Required Patents
To the best of our knowledge, there are no patents that would conflict with the incorporation of this MCP.

## References
(None.)
23 changes: 23 additions & 0 deletions chapters/lexicalstructure.tex
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,29 @@ \subsection{Strings}\label{strings}
\end{lstlisting}
\end{nonnormative}

\subsection{Units of Literal Constants}\label{units-literal-constants}

The following rules apply:
\begin{itemize}
\item The result of \lstinline!x + L!, \lstinline!L + x\lstinline!, \lstinline!x - L!, \lstinline!L - x!, \lstinline!x*L!, \lstinline!L*x!, \lstinline!x/L!, where \lstinline!x! is an expression with non-empty \lstinline!unit! attribute string `<s>` and \lstinline!L! is an expression containing only literal constants, shall have the same \lstinline!unit! attribute string of \lstinline!x!.
Copy link

@bilderbuchi bilderbuchi Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a proposal to resolve the distinction between built-in (exp()) and other (specificEnthalpy()) functions. In effect, we have to replace "expression containing only literal constants" with "expression containing only literal constants and mathematical operations".
Because there's some context to write up around this, see the separate gist https://gist.github.com/bilderbuchi/317bc1a2e039f477462a61f5826260ea

I also collected all the test cases I could find so far in one place, so we could cross-check against them (and maybe add to a test suite later -- ModelicaTest?): https://gist.github.com/bilderbuchi/317bc1a2e039f477462a61f5826260ea#file-mcp-0027-test-examples-md

I started a new discussion thread because the previous one grew huge and hard to view.
Feedback welcome!

Copy link

@bilderbuchi bilderbuchi Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@casella I'd like to hear thoughts on this approach (BB2a, BB2b, respectively) to express the distinction between builtin and other functions?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HansOlsson @henrikt-ma @casella it would be great to get your feedback on my comment https://github.com/modelica/ModelicaSpecification/pull/3266/files#r1049500278 with a proposed approach to resolve the distinction between built-in (exp()) and other (specificEnthalpy()) functions, and hopefully make progress on this PR. :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that built-in functions should be treated uniformly with other functions as much as possible, and we need to distinguish it in some other way. Specifically specificEnthalphy is a function where the interface-variables have non-empty units, and that should be preserved. Note that some built-in functions have unit (specifically for Angles) in Modelica.Math, which is kind of identical to having unit="1".

So in one sense many built-in functions can be seen as having unit="1" meaning that we can limit literal-part to functions where the interface is without unit. However, having an interface for built-in functions will stumble on abs, sign, sqrt, atan2, etc where it makes more sense to say that the unit-handling is defined according to the mathematical rules.

Additionally for deriving units it makes sense that each function call is treated separately; so that if a user defined their own abs-function it is possible to have different units for each call. With such a rule we can just exclude function calls from the literal part; as x+myabs(2) will be a two-step procedure: myabs#1.input=2, x+myabs#1.output.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the prompt feedback!

I believe that built-in functions should be treated uniformly with other functions as much as possible, and we need to distinguish it in some other way.

This seems like a reasonable aspiration to me! However, I caution that, to get this proposal over the finish line, Francesco (IMO rightly so) relentlessly tries to limit the scope of the proposal to dealing with the "wildcard effect" of literals only. Therefore, a more uniform treatment should maybe be built out elsewhere/after this is merged?

So in one sense many built-in functions can be seen as having unit="1"

Logically/"in a sense" as you say, I agree, but is this specified already?

meaning that we can limit literal-part to functions where the interface is without unit. However, having an interface for built-in functions will stumble on abs, sign, sqrt, atan2, etc where it makes more sense to say that the unit-handling is defined according to the mathematical rules.

As the present challenge revolves around "literals+function calls" only (i.e. an "expression of type L"), I don't think the mathematical rules (e.g. relating input and output units of sqrt) do not come into play at all?
A couple of lines down in the present PR diff, those functions are specifically limited to "dimensionless" inputs.

Additionally for deriving units it makes sense that each function call is treated separately; so that if a user defined their own abs-function it is possible to have different units for each call.

I'm not sure what part of my proposal touches upon that (or precludes different units for different calls)? I'm confused.

I'm not aware (but I'm not a specialist on that) that such unit semantics (output unit == input unit) are expressible with current Modelica for non-built-in functions?

With such a rule we can just exclude function calls from the literal part; as x+myabs(2) will be a two-step procedure: myabs#1.input=2, x+myabs#1.output.

I think (but don't remember exactly anymore) this two-step procedure is basically the wider issue of composing an expression (and its ultimate unit) from a tree of sub-expressions (and their units), I think as mainly @henrikt-ma worked on. The present PR tries to deal with one type of expression only (resulting unit of an expression involving literals), not the general case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to say sorry, I recognize your points myself. I understand that a conclave-style workshop would be preferable, but I'm not sure if such an approach is in the cards?

However, IIRC the way it seemed to me at least was that this proposal was near ~consensus (or readyness for review), and most conversation threads concluded. The main remaining item was I think to resolve the precision/scope of the "expression containing only literal constants" formulation, which I attempted again back in December with this Gist.

Copy link

@bilderbuchi bilderbuchi Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I'm not even sure how could I actually read the current proposal. Is there a way to get this branch compiled into html or PDF?

The CI system compiles a PDF, you have to navigate to the build checks below, on the Jenkins page hit the "ARtifacts" button in the upper right, and download the pdf. In this case, this link: https://test.openmodelica.org/jenkins/job/ModelicaAssociation/job/ModelicaSpecification/job/PR-3266/23/artifact/MLS.pdf (page 13)
Pretty neat I have to say, hats off to the MA dev that set this up! :-)

Copy link

@bilderbuchi bilderbuchi Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To summarize the situation hopefully more simply (from my point of view/recollection), and maybe spare you a re-read:

  • What we have already reached full consensus on right now is the desired behaviour of unit checking while avoiding the "wildcard effect", as codified in the set of unit-test-like examples that I collected from the discussions in one place here. If all else from this PR should be "lost", I hope that this set of examples is preserved to jump-start a future iteration on this topic.
  • What remains is to find out the/a minimal change to the spec that corresponds to that desired behaviour.
  • Your PR here is an attempt to formulate such a change.
  • The one thing your PR does not yet cover correctly is the last example in the section https://gist.github.com/bilderbuchi/317bc1a2e039f477462a61f5826260ea#proper-functions (as called out in that section).
  • My proposal update BB2a rectifies this deficiency AFAICT.
  • I'm asking if this proposal BB2a is acceptable in content/principle. If yes, I also ask if the refactored formulation BB2b, that streamlines the whole diff, is agreeable. I consider the second point optional if it turns out to be contentious.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it was pointed out below in #3266 (comment), the language group is not ready to incorporate a solution to the wildcard effect in isolation. Instead, it seems that what we have learned in these discussions need to be carried over to a fresh start on unit checking where we try to break down the problem into smaller pieces, and where we in the end will be able to see the complete picture of unit checking before merging anything to the master branch.

Copy link

@bilderbuchi bilderbuchi Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this recent comment in the notifications/PR discussion, thank you for that update!
After a couple of months of mostly productive back-and-forth iteration, and half a year of inactivity, that certainly feels like a disappointing outcome to me.
One can only hope that, after having failed to achieve sufficient consensus on this minimized-scope problem, making the scope of the problem larger will enable achieving that consensus somehow. This sounds unintuitive to me 😅, but if this is the way this works, hopefully such a holistic perspective will succeed in unblocking things for the language group. 🤞

I hope that at least the list of test cases we all came up with together will be useful during these future discussions.

\item The result of \lstinline!L/x\lstinline! shall have the \lstinline!unit! attribute string `1/(<s>)`.
\item The result of \lstinline!x^L! shall have the \lstinline!unit! attribute string of \lstinline!product(fill(x, L))! if L is a positive Integer, or the unit of \lstinline!1/product(fill(x, L))! if L is a negative Integer.
\item If either side of a relational operator is a literal constant, then it is assumed to have the same \lstinline!unit! attribute string of the other side, if that is well-defined.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest "If either side of a relational operator is an expression containing only literal constants" to mirror the formulation in rule 1. Otherwise T > (5+3) does not match this rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we find something better than "expression containing only literal constants", that is 😅

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. But step 1 is to use a consistent formulation in all rules, to then update it in step 2. :-)

\end{itemize}

The inputs of the elementary mathematical functions defined in \cref{built-in-mathematical-functions-and-external-built-in-functions} such as \lstinline!sin()!, \lstinline!cos()!, \lstinline!exp()!, etc., should have a dimensionless unit (e.g., \lstinline!"1"! or \lstinline!"rad"!) if the unit string of the input is non-empty. Function \lstinline!atan2(y,x)! should be treated as \lstinline!atan(x/y)! for dimensional consistency checking purposes.
In that case, their outputs are also implicitly assumed to have a dimensionless unit.

\begin{nonnormative}
Rationale: by default, literal Real and Integer constants do not have a defined, non-empty \lstinline!unit! string attribute; hence they act as "unit wildcards", preventing dimensional consistency checking of equations that contain them.
The rules regarding multiplication and division prevent this effect, allowing, e.g., to determine that \lstinline!v = sqrt(2*g*h)! is dimensionally consistent, while \lstinline!v = sqrt(2*g)! is dimensionally inconsistent, and thus most likely wrong.
The rules regarding addition and subtraction instead allow to perform some basic unit inference in expressions containing mixed literal constants and variables (when there are no ambiguities in doing so), again expanding the scope for dimensional consistency checking.
For example, they allow to determine that \lstinline!tau = L/(abs(v) + 1e-9)! is dimensionally consistent, while \lstinline!tau = 1/(abs(v) + 1e-9)! is not.


The rules involving elementary mathematical functions extend the scope of this concept to also allow determining that equations such as \lstinline!i = i0*exp(v/i0)! or \lstinline!i = v0*exp(i/i0)! are dimensionally inconsistent.
\end{nonnormative}

\section{Operator Symbols}\label{operator-symbols}

The predefined operator symbols are formally defined on page \pageref{lexical-conventions} and
Expand Down