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

Providing experiment annotation always override base class StartTime #3450

Closed
henrikt-ma opened this issue Nov 22, 2023 · 5 comments · Fixed by #3448
Closed

Providing experiment annotation always override base class StartTime #3450

henrikt-ma opened this issue Nov 22, 2023 · 5 comments · Fixed by #3448
Milestone

Comments

@henrikt-ma
Copy link
Collaborator

This is a follow-up to #2999, since I'm not happy with my conclusion about the consequences in #2985 (comment).

The problem is this part of the synopsis of experiment:

  Real StartTime(unit = "s") = 0;

because it means it is not possible to inherit StartTime from a base class.

In order to properly support inheritance of experiment options, I think we have to move the default value for StartTime out of the synopsis, and instead describe it in the semantics part.

@HansOlsson
Copy link
Collaborator

Note that inheritance of experiment is part of #2535 and has not yet been merged.

I agree that it should be possible to inherit a modified StartTime, but I don't think the consequences are like that.
Clearly it must be clarified, to avoid ambiguities.

I view experiment as similar to a parameter record in the class, and modifiers as modifiers - not as record constructors.

If we include the parameter record we see the similarity:

model A
  Experiment experiment2(StartTime=-1.0);
  annotation(experiment(StartTime = -1.0));
end A;

model B
  extends A(experiment2);
  /* This model has StartTime = -1.0, inherited from A. */
  annotation(experiment); /* Not a valid experiment annotation?; as a modifier it modifies nothing */
end B;

model C
  extends A(experiment2());
  /* This model also has StartTime = -1.0, inherited from A. */
  annotation(experiment()); /* Valid annotation. */
end C;

model D
  extends A(experiment2=Experiment()); /* Ignores StartTime value */
  annotation(experiment=Experiment()); /* Should not be valid */
end D;

It may be that we should change it to record Experiment ... end Experiment;parameter Experiment experiment; to make it clearer, and possibly state that the name Experiment is internal.

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Nov 22, 2023

Sorry, I forgot that #2535 isn't merged yet. Anyway, I think we have identified an important ambiguity regarding the meaning of annotations defined by pseudo-code records. Better than record Experiment … end Experiment; parameter Experiment experiment would be to explain in the Notation section what it means to define an annotation using record syntax, and then link to this explanation from the introduction to Annotations.

@HansOlsson
Copy link
Collaborator

HansOlsson commented Dec 21, 2023

Sorry, I forgot that #2535 isn't merged yet. Anyway, I think we have identified an important ambiguity regarding the meaning of annotations defined by pseudo-code records. Better than record Experiment … end Experiment; parameter Experiment experiment would be to explain in the Notation section what it means to define an annotation using record syntax, and then link to this explanation from the introduction to Annotations.

That is the idea in #3448 except that it is explained in the annotation-chapter; instead of at the start (and then linked) - to hopefully make it slightly easier to find, and having a less overwhelming start of the specification.

I hope that PR will close this issue.

@HansOlsson
Copy link
Collaborator

As far I understand #3448 will resolve this.

@HansOlsson HansOlsson linked a pull request Aug 30, 2024 that will close this issue
@henrikt-ma
Copy link
Collaborator Author

As far I understand #3448 will resolve this.

Yes, this is also my understanding; the important part is the clarification that annotations shall be seen as modifications, not record construction with defaults.

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