-
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
Start clearing up annotation syntax for functions. #3617
base: master
Are you sure you want to change the base?
Conversation
I updated the description to make it a link to the issue #3495 for an easy lookup. |
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.
Suggesting minor change that will make this more compatible with #3621.
Co-authored-by: Henrik Tidefelt <[email protected]>
chapters/functions.tex
Outdated
\begin{synopsis}[grammar]\begin{lstlisting} | ||
"LateInline" "=" ( false | true ) | ||
\begin{synopsis}\begin{lstlisting} | ||
/*literal*/ constant Boolean LateInline = false; |
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 thought we decided to avoid writing out defaults like this, as it gives the impression that LateInline = false
is what one is supposed to write?
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.
As I recall the idea was not use the form when specifying the default literal value (assuming it exists), when it is so "normal" that specifying it would look like an error (like singleInstance).
Different inlining variant and GenerateEvents don't fall under that; and is more similar to absoluteValue.
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 not how I recall it. For me, the main problem which was resolved when removing all those defaults was that what the reader sees first shouldn't be something one normally doesn't write when using the annotation. This was avoided by drastically reducing the use of declaration equations for explaining defaults in case of component style annotations.
I find LateInline
similar to DocumentationClass
, for which the default was removed in aa6ce47.
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 me these two explanations are fairly similar.
We agree that using DocumentationClass = false
does not really make sense at all in practice, the point is that LateInline = false
is just a bit unusual - one can easily see someone experimenting with setting LateInline to true or false to test the result.
And most of the removed defaults in that commit weren't literal values, but explanations as defaults.
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.
OK, so we have different recollections of the exact formulations used in the meeting discussions behind removing literal defaults. Fine.
Now, if we think about how to write clear specification text, don't you agree that there is a problem with picking the style of presentation based on vague opinions about what is more or less unusual or odd? … that specifying defaults for component style annotations sometimes with declaration equation and sometimes in text (even when the default corresponds to a literal value), is causing more confusion than useful brevity?
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.
But we have lots of annotations with defaults, so introducing a rule saying that we shouldn't use defaults doesn't make sense.
The following are some annotations that are defined with defaults: Documentation.info, Documentation.revisions, Documentation.figures (and many sub-elements), Documentation.styleSheets, experiment.StartTime, and for graphical items origin, rotation, preserveAspectRatio, initalScale, visible, primitivesVisible, fontSize, textColor, color, pattern, ....
Thinking more it might be that we should reconsider the previous idea of not using defaults in some 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.
There's a difference between component style annotations and record style annotations. As far as I can see, all of your examples are within such records:
Documentation
,experiment
,GraphicItem
,CoordinateSystem
,Text
The point I am raising in this thread is about component style annotations.
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 afraid defaults should be presented in the text rather than as a default binding.
Language group: Describe these in text (not as default), and then later revisit all. |
Also provide smarter default for LateInline.
This has now been implemented. @henrikt-ma |
@@ -1719,6 +1722,8 @@ \section{Function Inlining and Event Generation}\label{function-inlining-and-eve | |||
Late inlining is especially useful for differentiation and inversion of functions; for efficiency reasons it is then useful to replace all function calls with identical input arguments by one function call, before the inlining. | |||
\end{nonnormative} | |||
|
|||
The default for late inlining is tool-specific, in particular tools may automatically delay inlining to differentiate or possible invert the function before inlining 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.
Above, the default was described after giving the meaning of Inline = false
. It would make sense to be consistent and describe the default after the meaning of LateInline = false
.
@@ -1719,6 +1722,8 @@ \section{Function Inlining and Event Generation}\label{function-inlining-and-eve | |||
Late inlining is especially useful for differentiation and inversion of functions; for efficiency reasons it is then useful to replace all function calls with identical input arguments by one function call, before the inlining. | |||
\end{nonnormative} | |||
|
|||
The default for late inlining is tool-specific, in particular tools may automatically delay inlining to differentiate or possible invert the function before inlining 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.
Trying to fix some language:
The default for late inlining is tool-specific, in particular tools may automatically delay inlining to differentiate or possible invert the function before inlining it. | |
The default for late inlining is tool-specific. | |
In particular, tools may automatically delay inlining in order to take advantage of function annotations for derivatives and inverses. |
\end{lstlisting}\end{synopsis} | ||
\begin{semantics} | ||
Has only an effect within a function declaration. | ||
|
||
If {\lstinline!true!}, the model developer proposes to inline the function after the function is differentiated for index reduction, and before any other symbolic transformations are performed. | ||
This annotation cannot be combined with annotations {\lstinline!Inline!} and {\lstinline!LateInline!}. | ||
The default is to not perform this specific inlining. |
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.
How about swapping order with the previous sentence, and then have a paragraph break before the rule about combining annotations?
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.
Basically good, but there was at least one need for improved language.
It is intended to work towards close #3495 - but it doesn't handle all of the annotations as some are problematic.
Note that I deliberately added default values in some cases, I believe they are the correct ones.
It might be that we should add more, or view this as enough to close it - and later think about the rest.
The problem with the other ones are:
smoothOrder(normallyConstant=x)=2
(I have might have missed some that could be handled.)