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

Define more annotations as declarations #3448

Merged
merged 40 commits into from
Nov 6, 2024

Conversation

HansOlsson
Copy link
Collaborator

More consistent non-syntax variant of defining annotations, based on comments in #2999

Notes:

  • A remaining issue is whether they (and experiment) should be parameter, constant, or evaluable. That may be easier to handle in a separate PR.
  • DocumentationClass was previously syntactically incorrect as grammar.
  • It is consistently using mathit for the message to replace, since that also works in inline code (and non-inline if remembering mathescape=true...)
  • Two of the annotations are as previously only defined for value true, DocumentationClass and singleInstance. It may be best to remove the value, but in that case the text needs to be reformulated.
    • The absoluteValue was previously defined as =false in the syntax, but the text explained =true as well.
    • The defaultConnectionStructurallyInconsistent was previously defined as =true in the syntax, but the text specified "if true".
  • The Icon and Diagram-layer already had both the old and this variant. In 18.6 they are defined with grammar-syntax, and then in 18.6.1.1 as records.

@maltelenz
Copy link
Collaborator

This seemingly opens up for expressions instead of the literals true or false for some of these. Does it? Do we want to?

@HansOlsson
Copy link
Collaborator Author

This seemingly opens up for expressions instead of the literals true or false for some of these. Does it? Do we want to?

Good point, and the answer to the latter is it depends
For some of them it makes sense to have it depend on an evaluable parameter, for others not; see previous discussion in #3375

@HansOlsson
Copy link
Collaborator Author

This seemingly opens up for expressions instead of the literals true or false for some of these. Does it? Do we want to?

Good point, and the answer to the latter is it depends For some of them it makes sense to have it depend on an evaluable parameter, for others not; see previous discussion in #3375

One possibility would be to change them to constant and parameter respectively, and state that constants must be given literal values and parameters must be evaluable.

@HansOlsson
Copy link
Collaborator Author

This seemingly opens up for expressions instead of the literals true or false for some of these. Does it? Do we want to?

Good point, and the answer to the latter is it depends For some of them it makes sense to have it depend on an evaluable parameter, for others not; see previous discussion in #3375

One possibility would be to change them to constant and parameter respectively, and state that constants must be given literal values and parameters must be evaluable.

I have now included that definition.

  • I have deliberately not added it for other annotations, the idea is to add that in separate PRs once we have defined what it means. (So that we separately can discuss which ones must be evaluable or not after we have defined what it means at all.)
  • The reason the Documentation-record was anyway updated (even though it wasn't touched by this PR previously) was that it seemed logical to have the common definition at the start of the chapter, the Documentation-definition relied on the unstated logic for records, and I believe it is fairly uncontroversial that the info-text shall be a literal.
  • I use plain parameter for an evaluable parameter to keep it short; and similarly constant for literal.

Note in particular that the Figure-annotation isn't a parameter at all - since it directly refers to expressions for curves. (But the rest of it should likely be constant (or possibly parameter)).

@HansOlsson
Copy link
Collaborator Author

I would like some more input on this:

  • Is it a good approach in general?
  • Is it good to have the definition of parameter/constant and record at the start of the chapter?
  • Is "misusing" parameter/constant for annotations a good idea? I know that we could use parameter ... annotation(Evaluate=true); to avoid one of the issues - but it will make the lines longer and harder to read.
  • I similarly know that we can avoid the record-issue by having a differently named record class, and then a record component. That would be more correct, but more text to read. If some prefer the more correct approach I think we can discuss that in a separate PR. (No such record is added in this PR, it just describes how the existing ones work.)
  • Can we merge this one as is, and then later discuss individual annotations? (Including which ones should be literal or evaluable parameters?)

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 think we need to be more careful to not overload parameter and constant with other than the usual meanings.

This doesn't mean that I'm pushing for having, say, more constant than "literal" variability, but overall I would actually expect it to be possible to ease up variability requirements in the future to have less ad-hoc requirements to only support constant expressions in the form of literals.

chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
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 we still need to iterate on the general principles a bit more.

chapters/annotations.tex Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
HansOlsson and others added 2 commits October 6, 2024 23:13
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
@HansOlsson
Copy link
Collaborator Author

@henrikt-ma I believe all issues are now resolved for this PR; there could be another PR for clarifying why absoluteValue is a good idea for temperatures.

@HansOlsson HansOlsson dismissed henrikt-ma’s stale review November 6, 2024 10:30

Think everything was resolved, and waited weeks for new feedback. The PR is also getting difficult to handle in github due to the growing number of comments. so better to add new PR for remaining issues

@HansOlsson HansOlsson merged commit 2c2df56 into modelica:master Nov 6, 2024
1 check passed
@HansOlsson HansOlsson deleted the AnnotationVars branch November 6, 2024 10:31
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.

Providing experiment annotation always override base class StartTime
4 participants