Define experiment using a record. #2999
Conversation
There was a problem hiding this comment.
As noted in the comment in #2985, we have other places where we define annotations using grammar style. Is that acceptable to keep grammar-styled definitions? Or do we need to update them all? It probably deserves its own ticket. But it is also relevant to my comment on line 389. Should we mix them up or should we move away from grammar style completely? There are some annotations that are not records. For example, HideResult that follows right after experiment would need to be defined as a type, I guess.
There was a problem hiding this comment.
In addition to comments on the changes themselves, we should also get a formal decision on #2985 before going ahead with the core change of disallowing the argument-less annotation.
From review Co-authored-by: Henrik Tidefelt <[email protected]> Co-authored-by: Elena Shmoylova <[email protected]>
|
Note that the important part is that now it is clear that StopTime must be specified, but the others have defaults (0 for StartTime - which matters, Tolerance and Interval are less critical). That is consistent with the use in MSL where some examples only specify StopTime, some StopTime and Interval, and some StopTime Interval and Tolerance. |
There was a problem hiding this comment.
In general, I like the changes. But I feel like there is a point missing. Maybe it's the conflict between the intent of the experiment annotation the way it is defined in the specification and the way I see it being used in practice. Specifically, what I see in the MSL, for example, is that the presence of the experiment annotation with specified StopTime indicates that this is an example and StopTime, etc are the simulation parameters. In general, I see the presence of the annotation meaning that the model can be used as a stand-alone model for simulation. If there is no such annotation, it is does not mean it cannot be used a stand-alone simulation model but there is no guarantee that it has any meaning (like model of a resistor is not really a complete simulation model).
This meaning of the presence of the experiment annotation is not currently reflected in the Specification. And from that, there is also the question what it means if somebody specified experiment but did not specify StopTime (which was the point of #2985). The precedence in the MSL is that it is not considered an example model. According to the current text of the PR, StopTime must be specified. Does it mean an error must be given if the model is used a stand-alone? Also, if it is an error to specify the experiment without StopTime, what if somebody tries to simulate a model that does not have the experiment specified at all? In practice, tools usually simulate such models and fill out all the fields of the experiment including StopTime with tool-specific defaults. So why should the case of experiment without StopTime be different?
There was a problem hiding this comment.
I don't think we should require StopTime in general, only in a merged experiment annotation after inheritance, as discussed in my preferred alternative in #2314 and #2535 .
We need to resolve that discussion together with this, or write this one in a way that doesn't prevent the flexible solution of merged inheritance.
There was a problem hiding this comment.
Basically the same concern about requiring StopTime as expressed by other reviewers.
Edit: By the way, I think we can work this out in parallel and independently of #2314 and #2535. We just need to leave the same room for interpretation as before, and then rely on #2535 for defining what it really means that StopTime is defined in a model.
I'd say no; it just means that from the specification's point of view the model isn't a simulation model. Maybe we don't need to specify anything further for this case and leave it up to tools to decide what to do? |
Co-authored-by: Elena Shmoylova <[email protected]>
Co-authored-by: Henrik Tidefelt <[email protected]>
I agree with this general idea - but as I see it there are two possibilities for stating this requirement. After some thinking I believe that we should use experiment.StopTime; since it seems that otherwise we have introduce an error case (experiment without StopTime) just to annoy users.
This is now handled (although I think those models should be corrected). |
In order to handle that in the future I ensure that the statement for StopTime requires that it is set; not modified. |
Co-authored-by: Henrik Tidefelt <[email protected]>
They will be added later in a different PR, and shouldn't block this one.
This is a non-constructive blocker. You first pushed for adding the relation between StopTime and simulation model, and when explaining the current status quo (as it is used in e.g. MSL) you then try to block the PR.
|
I use the same introductory text as Documentation, in order to clarify that the annotation has the same name as the record.
|
@maltelenz @eshmoylova @henrikt-ma please redo the reviews. |
There was a problem hiding this comment.
I factored out the relation between simulation models and models with components of partial type: #3435
I also factored out the part regarding simulation models and StopTime: #3436
With these topics out of the way, I just see the need to remove running text from the synopsis, and ideally add a unit for Tolerance.
Co-authored-by: Henrik Tidefelt <[email protected]>
There was a problem hiding this comment.
My concern about inheritance seems taken care of, but I'll leave more detailed reviewing to @henrikt-ma's capable hands.
There was a problem hiding this comment.
Looks OK.
Now that the annotation isn't defined in terms of UNSIGNED-NUMBER, this raises the question of expression variability… Perhaps we should open another PR about specifying which of these are parameter (evaluable/constant) expressions before we forget about it?
Yes. But in a bit, since I think we should first erase the other unneeded uses of grammar in that chapter. |
Remove the empty experiment and define it using a record instead (per discussions).
Closes #2985