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.

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

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

Conversation

HansOlsson
Copy link
Collaborator

@HansOlsson HansOlsson commented Nov 29, 2024

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:

  • "Trivial": the external function ones can specify either an array or a scalar.
  • "Messy": The derivative and inline-annotations don't fit into the pattern for two reasons:
    • The "value" is not a value - so no good type for the declaration (it is a function for derivatives, and something else for inverse)
    • The optional sub-modifiers for values, like smoothOrder(normallyConstant=x)=2

(I have might have missed some that could be handled.)

@eshmoylova
Copy link
Member

I updated the description to make it a link to the issue #3495 for an easy lookup.

@HansOlsson HansOlsson added this to the 2024-Decemeber milestone Dec 4, 2024
Copy link
Collaborator

@henrikt-ma henrikt-ma left a 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.

Copy link
Collaborator

@henrikt-ma henrikt-ma left a 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.

@HansOlsson HansOlsson requested a review from henrikt-ma January 3, 2025 14:17
@HansOlsson HansOlsson modified the milestones: 2024-December, 2025-January Jan 8, 2025
@HansOlsson
Copy link
Collaborator Author

Language group: Describe these in text (not as default), and then later revisit all.

Also provide smarter default for LateInline.
@HansOlsson
Copy link
Collaborator Author

Language group: Describe these in text (not as default), and then later revisit all.

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.

Copy link
Collaborator

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.
Copy link
Collaborator

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:

Suggested change
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.

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, handled both of the ones above.

\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.
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be done now.

Copy link
Collaborator

@henrikt-ma henrikt-ma left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notation used for function annotations will soon be obsolete
3 participants