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

Gather annotations for functions with impact on symbolic processing #2895

Closed
henrikt-ma opened this issue Mar 15, 2021 · 8 comments · Fixed by #2922
Closed

Gather annotations for functions with impact on symbolic processing #2895

henrikt-ma opened this issue Mar 15, 2021 · 8 comments · Fixed by #2922
Milestone

Comments

@henrikt-ma
Copy link
Collaborator

We've recently made improvements to the inverse and derivative annotations for functions, and I think this makes it a good time to also consider the other highly related annotations for functions with impact on symbolic processing. Currently, these are described in 18.3 Annotations for Code Generation:

  • Inline
  • LateInline
  • InlineAfterIndexReduction
  • GenerateEvents
  • smoothOrder

As is clearly stated in the text, smoothOrder is closely related to derivative:

A function declaration can have an annotation \fmtannotationindex{derivative} specifying the derivative function or preferably, for a function written in Modelica, use the \lstinline!smoothOrder! annotation to indicate that the tool can construct the derivative function automatically, \cref{annotations-for-code-generation}.

To me, this just shows that smoothOrder and derivative should be described next to each other.

That the inlining annotations have a close relation with inverse and derivative should probably be expressed more clearly both in text and by describing the annotations in the same chapter and section. When to inline is seems to me as more of a tricky tradeoff between keeping the specialized inverse and derivative functions applicable and making the expressions of a function body available to general symbolic processing, than a question about how to avoid function calling overhead in the generated code (referring to the title of 18.3).

GenerateEvents has an intricate relation to inlining. For example, the combination GenerateEvents = true and Inline = false is particularly challenging, showing the close relation between the two. Because of this, but also because GenerateEvents has other effects on symbolic processing as well, it should be described along with annotations for inlining as well as other annotations for functions with impact on symbolic processing.

If all of the above would be moved to a common section for annotations for functions (chapter 12 or 18), this would leave only Evaluate and HideResult in 18.3, neither of which seems related to code generation. Thus, to make a more concrete proposal, I suggest:

  • Moving smoothOrder to 12.7 Declaring Derivatives of Functions
  • Introducing a new section, Function Inlining and Event Generation after 12.8, where we place Inline, LateInline, InlineAfterIndexReduction, and GenerateEvents.
  • Moving HideResult to 18.4 Annotations for Simulations
  • Evaluate is so fundamental that it deserves its own section right at the beginning of chapter 18: Annotations of Utter Importance. :)
@HansOlsson
Copy link
Collaborator

The first two changes makes sense to me; as those annotations are for functions and it makes sense to keep function annotations together.

However, I think that HideResult and Evaluate can stay in the now shorter "Annotations for Code Generation". I don't think we should create a separate section for "Evaluate" just because it is that important, and "Annotations for Simulations" are about general settings for the entire simulation, whereas HideResult, as well as Evaluate, influence the code for specific components.

@HansOlsson HansOlsson added this to the Phone2021-2 milestone Mar 16, 2021
@HansOlsson
Copy link
Collaborator

Agree on restructuring chapter, and move function ones to function chapter.

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Apr 29, 2021

Regarding GenerateEvents, why don't we simply deprecate it instead of trying to explain the intricate relation to function inlining, backed up by issues like these:

Edit: Here is a proposal for a poll regarding GenerateEvents:

  • Do nothing.
  • Make Inline = true implied, and don't allow combination with any other inlining annotation.
  • Just Deprecate.
  • Make Inline = true implied as above, plus deprecation.

@henrikt-ma henrikt-ma modified the milestones: Phone2021-2, Phone2021-3 Apr 29, 2021
@henrikt-ma
Copy link
Collaborator Author

Adding to milestone of upcoming phone meeting in hope of discussion the question above regarding deprecation of GenerateEvents.

@HansOlsson
Copy link
Collaborator

Regarding GenerateEvents, why don't we simply deprecate it instead of trying to explain the intricate relation to function inlining, backed up by issues like these:

The issue is that people have found it useful, and removing it without an alternative doesn't make sense.

So, the obvious question is whether we can solve it in another way - and one option would be to go for MCP-0012, https://github.com/modelica/ModelicaSpecification/blob/MCP/0012/RationaleMCP/0012/MCP-0012_CallingBlocksAsFunctions.pdf and instead of allowing functions to generate events we allow calling blocks that generate events as if they were functions.

An issue is that level 2 and 3 of that proposal rely on not yet unsolved issues regarding state-machines, but that's just another reason to improve state-machines.

@henrikt-ma
Copy link
Collaborator Author

See proposal in edited comment above. With Inline = true implied, we could actually present GenerateEvents as a form of inlining without treating all expressions as wrapped in noEvent(…).

@HansOlsson
Copy link
Collaborator

Regarding GenerateEvents, why don't we simply deprecate it instead of trying to explain the intricate relation to function inlining, backed up by issues like these:

Edit: Here is a proposal for a poll regarding GenerateEvents:

  • Do nothing.
  • Make Inline = true implied, and don't allow combination with any other inlining annotation.
  • Just Deprecate.
  • Make Inline = true implied as above, plus deprecation.

I don't see why the second option forbids combinations with other inlining. Both inlining after index reduction and late inlining should work together with generating events.

@henrikt-ma
Copy link
Collaborator Author

I don't see why the second option forbids combinations with other inlining. Both inlining after index reduction and late inlining should work together with generating events.

The reason was just for simplicity; the Inline = true is the easiest one to grasp, I believe, and formulating the rule also became easier by picking one specific of the inlining variants. So, yes, the poll is missing a more flexible alternative:

  • When GenerateEvents = true, only allow other inlining annotations to be used with a setting of true, and make Inline = true implicit in case no inlining annotation is given.

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 a pull request may close this issue.

2 participants