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

Inheritance of annotations #2314

Closed
casella opened this issue Feb 13, 2019 · 11 comments · Fixed by #2535
Closed

Inheritance of annotations #2314

casella opened this issue Feb 13, 2019 · 11 comments · Fixed by #2535
Labels
discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP)

Comments

@casella
Copy link
Collaborator

casella commented Feb 13, 2019

Section 7.1 of the specification says:

The elements of the flattened base class become elements of the flattened enclosing class, and are added at the place of the extends-clause: specifically components and classes, the equation sections, algorithm sections, optional external clause, and the contents of the annotation at the end of the class, but excluding import-clauses.

Regarding annotations, the specification is a bit vague, in the sense that it is not clear in general how annotations of the base class are merged with the annotations defined in the derived class.

The only case which is covered explicitly by the specification is graphical annotation, see Section 18.6:

The graphics is specified as an ordered sequence of graphical primitives, which are described below. First base-class contents is drawn according to the order of the extends-clauses, and then graphical primitives are drawn according to the order such that later objects can cover earlier ones.

Other than that, nothing at all is said. I think this should be clearly specified, since some annotations have an impact on the behaviour of the model (e.g. Evaluate, Inline, derivative, etc., as well as the experiment annotation for executable models)

@casella
Copy link
Collaborator Author

casella commented Feb 13, 2019

@perost, you may want to comment on that

@casella
Copy link
Collaborator Author

casella commented Feb 13, 2019

As a simple test case, consider this package

package P
  model M
    parameter Real x = 1;
  annotation(experiment(StopTime = 2));
  end M;

  model M2
    extends M(x = 2);
  end M2;
end P;

According to the specification, I would say that if I simulate M2, the simulation should last 2 seconds. In fact, both Dymola and OpenModelica ignore the experiment annotation in the base class and only simulate for one second.

@beutlich
Copy link
Member

beutlich commented Feb 14, 2019

As briefly discussed with @gkurzbach, one idea of the MCP0008 (Custom annotations) was to improve the behavior on inheritence. But one reason MCP0008 never got mature was the breaking backward-compatibility with existing annotations then.

@beutlich beutlich added MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP) discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended labels Feb 14, 2019
@HansOlsson
Copy link
Collaborator

Other than that, nothing at all is said. I think this should be clearly specified, since some annotations have an impact on the behaviour of the model (e.g. Evaluate, Inline, derivative, etc., as well as the experiment annotation for executable models)

I agree that it could be specified more clearly; but for some of these it seems odd to inherit the annotation, e.g.: you normally don't specify derivative of a base-function. And even if a base-function did specify a derivative it's odd that the function extending from it could still use it - as it seems as if something added would invalidate it. (Short class definitions, e.g., function foo=bar(x=2); are a bit special.)

@HansOlsson HansOlsson modified the milestones: Design99, ModelicaSpec3.5 May 15, 2019
@HansOlsson
Copy link
Collaborator

Language group:
All the annotations that shouldn't have been annotations probably shouldn't be inherited; like derivative.
The experiment-annotation would make sense to inherit.

But:

package P
  model M
    parameter Real x = 1;
  annotation(experiment(StopTime = 2));
  end M;

  model M2
    extends M(x = 2);
    annotation(experiment(Tolerance=1e-6)));
  end M2;
end P;

Should M2 have StopTime=2? (Reason for wanting this would be a base-experiment with solver and then specific experiments with e.g. different StopTime.)
Testing usually checks for experiment-annotation, which would require checking more. (Makes sense to anyway instantiate all models when testing.)
What if you have multiple base-classes with conflicting and/or disjoint experiment-annotation?

Need to consider more.

@HansOlsson
Copy link
Collaborator

If we view the experiment-annotation as completely overriding existing experiment-annotation (similarly as if it were a record value and experiment(Tolerance=1e-6) were a constructor), and have local experiment-annotation overriding inherited ones, then it can be introduced in a backwards compatible way.

The reason is that the inherited one will then only be used if there is no locally defined experiment-annotation, and the specification doesn't state what the default stop-time etc should be in that case.

@HansOlsson
Copy link
Collaborator

I see three possibilities:

  • Work on MCP0008 to handle annotations in general
  • A specific MCP for specifying what annotations are inherited etc.
  • Just specify that experiment-annotations are inherited, and can be overridden as an ad-hoc solution. (Might not need MCP).

@HansOlsson HansOlsson modified the milestones: ModelicaSpec3.5, Design101 Feb 12, 2020
@HansOlsson HansOlsson added the decided A decision has been made (label added before the spec is changed) label Feb 19, 2020
@HansOlsson
Copy link
Collaborator

I realize the notes are not clear here. Was the decision just to inherit experiment-annotation, or?

@HansOlsson HansOlsson modified the milestones: Design101, Phone2020-1 Mar 16, 2020
@henrikt-ma
Copy link
Collaborator

That's how I remember it.

@HansOlsson
Copy link
Collaborator

HansOlsson commented Mar 19, 2020

Poll:

  • Not inheriting at all: 0
  • As in Inherit experiment #2535, local experiment completely overrides: Hans, Stefan Z, Filippo
  • Local experiment partially overrides the one from first base-class that contain, usual modification merge: Henrik, Gerd, Stefan V
  • Local experiment overrides, but unspecified attributes are not specified (non-portable): Hans
    Tie, Postpone until next meeting.

@HansOlsson HansOlsson modified the milestones: Phone2020-1, Phone2020-2 Apr 7, 2020
@HansOlsson HansOlsson removed the decided A decision has been made (label added before the spec is changed) label May 19, 2020
@HansOlsson HansOlsson modified the milestones: Phone2020-2, Phone2020-4 Jul 10, 2020
@HansOlsson HansOlsson modified the milestones: Phone2020-4, Phone2020-5 Oct 26, 2020
@HansOlsson HansOlsson removed this from the Phone2020-5 milestone Dec 11, 2020
@HansOlsson HansOlsson added this to the Phone2020-6 milestone Dec 11, 2020
@HansOlsson
Copy link
Collaborator

Two best options in previous poll were:

  • As in Inherit experiment #2535, local experiment completely overrides
  • Local experiment partially overrides the one from first base-class that contain, usual modification merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants