Skip to content
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.

Sign up for GitHub

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

Make it explicit that tools can show the value of variables for asserts #3641

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Make it explicit that tools can show the value of variables for asserts #3641

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8097dec
Make it explicit that tools can show the value of variables.
HansOlsson Jan 23, 2025
a5750c8
ExtendRecommendations
HansOlsson Jan 24, 2025
6dd39db
AlsoCondition
HansOlsson Jan 24, 2025
41adb86
Update chapters/equations.tex
HansOlsson Feb 6, 2025
adc1c62
Update chapters/equations.tex
HansOlsson Feb 6, 2025
ad62cc0
Reformulate to be clearer what is recommended for tools and users.
HansOlsson Feb 11, 2025
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
5 changes: 5 additions & 0 deletions chapters/equations.tex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ \subsection{assert}\label{assert}
\lstinline!condition! needs to be implicitly treated with \lstinline!noEvent! since otherwise events might be triggered that can lead to slightly changed simulation results.
\end{nonnormative}
\end{itemize}
Tools may show more than the message for failed assertions, in particular the condition and the values of any variables used in it.
HansOlsson marked this conversation as resolved.
Show resolved Hide resolved

\begin{nonnormative}
The \lstinline!AssertionLevel.error! case can be used to avoid evaluating a model outside its limits of validity; for instance, a function to compute the saturated liquid temperature cannot be called with a pressure lower than the triple point value.
Expand All @@ -518,6 +519,10 @@ \subsection{assert}\label{assert}
AssertionLevel.warning);
assert(T > 200 and T < 500, "Medium model outside feasible region");
\end{lstlisting}

It is recommended that models have a simple message as above and not include the value of any variables (like \lstinline!T!) in the message given to \lstinline!assert!.
HansOlsson marked this conversation as resolved.
Show resolved Hide resolved
Tools are (especially for simple messages) recommended to automatically include the values of relevant variables, as part of the log or interactively accessible.
Similarly tools are recommended to include the failed condition.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already part of the normative text above, so it needs to be removed or reformulated with the user in focus:

Suggested change
Tools are (especially for simple messages) recommended to automatically include the values of relevant variables, as part of the log or interactively accessible.
Similarly tools are recommended to include the failed condition.
In this case, for example, embedding the value of \lstinline!T! in the message is likely to result in duplicated information and should be avoided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea with "especially for simple messages" was to allow tools to skip the duplicated T in such cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically to allow good compatibility with existing models that already have such complicated messages, but provide a simple recommendation for users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, tools may be smarter for a transition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has now been reformulated to be clearer (for tools and users) and include the point about duplicate information.

\end{nonnormative}

\subsection{terminate}\label{terminate}
Expand Down