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

Standardise Evaluate = true handling on records #2288

Closed
dietmarw opened this issue Dec 3, 2018 · 21 comments · Fixed by #2771 or #2779
Closed

Standardise Evaluate = true handling on records #2288

dietmarw opened this issue Dec 3, 2018 · 21 comments · Fixed by #2771 or #2779
Milestone

Comments

@dietmarw
Copy link
Member

dietmarw commented Dec 3, 2018

The newest Dymola version 2019FD01 now handles the Evaluate = true annotation as follows:

Annotation Evaluate=true on records or model components take precedence over annotation Evaluate=false on single parameters

If you use the annotation annotation(Evaluate=true) on a record or model component
all corresponding parameters will be evaluated even if some of them individually have the
annotation annotation(Evaluate=false) . This means that you do not have to add the
annotation annotation(Evaluate=true) to each individual parameter of a record or
model component to evaluate it.
Note that this does not apply if you use the annotation annotation(Evaluate=false)
on a record or model component; this annotation will not override any annotation of any
corresponding individual parameter.

That should probably be standardised to avoid user models behaving differently in future.

@HansOlsson HansOlsson added this to the Phone2020-6 milestone Dec 11, 2020
@HansOlsson
Copy link
Collaborator

Clarifying existing rule on Evaluate.
Agreement.

HansOlsson added a commit to HansOlsson/ModelicaSpecification that referenced this issue Dec 16, 2020
HansOlsson added a commit that referenced this issue Dec 16, 2020
* Specify Evaluate for hierarchical components.
Closes #2288
@henrikt-ma
Copy link
Collaborator

In the meeting (#2288 (comment)), it was said that priority would not be given to Evaluate = true, but this is how it still ended up in #2771.

I also find the both used in this added sentence somewhat unclear:

In the case of hierarchical components it is applied to all components, and if both of them have annotations then \lstinline!Evaluate = true! takes precedence.

I get the point of it being applied to all components, but would find it more consistent with the sentence before if the outermost is the one in effect also in this case (and this is how I think it was presented in the meeting).

@henrikt-ma henrikt-ma reopened this Dec 16, 2020
@henrikt-ma
Copy link
Collaborator

While it would certainly be possible to make a poll regarding the precedence of Evaluate = true vs Evaluate = false, I think we should't forget what the underlying problem is here, that has been noted many times in the past:

What we really need is a concept for Evaluate that can be controlled by some sort of modification (without having to introduce a structural parameter in the annotation), so that you can have Evaluate = false on a hierarchical component, and then override this with Evaluate = true on individual record members.

@HansOlsson
Copy link
Collaborator

In the meeting (#2288 (comment)), it was said that priority would not be given to Evaluate = true, but this is how it still ended up in #2771.

I also find the both used in this added sentence somewhat unclear:

In the case of hierarchical components it is applied to all components, and if both of them have annotations then \lstinline!Evaluate = true! takes precedence.

I get the point of it being applied to all components, but would find it more consistent with the sentence before if the outermost is the one in effect also in this case (and this is how I think it was presented in the meeting).

Hmm... I had to investigate this more here.

It seems the reason Evaluate=true is giving preference is that there were a number of existing models that had this combination (I don't know why Evaluate=false was added in them as it didn't have any effect earlier).
We cannot quickly investigate the implication of changing that so the realistic possibilities:

  • Go with the decision.
  • Revert adding this to Modelica 3.5 and investigate it further for a future release.

@henrikt-ma
Copy link
Collaborator

It seems the reason Evaluate=true is giving preference is that there were a number of existing models that had this combination (I don't know why Evaluate=false was added in them as it didn't have any effect earlier).

Just to make sure there is no confusion here. Do you really mean that these are components with Evaluate = false, having children with Evaluate = true?

@HansOlsson
Copy link
Collaborator

It seems the reason Evaluate=true is giving preference is that there were a number of existing models that had this combination (I don't know why Evaluate=false was added in them as it didn't have any effect earlier).

Just to make sure there is no confusion here. Do you really mean that these are components with Evaluate = false, having children with Evaluate = true?

Yes, as far as I recall, components with Evaluate=false where sub-components have Evaluate=true.

@henrikt-ma
Copy link
Collaborator

  • Go with the decision.

The point of reopening was that the decision was based on a discussion where it was clarified that this sort of precedence rule wouldn't be introduced.

@henrikt-ma
Copy link
Collaborator

I don't know why Evaluate=false was added in them as it didn't have any effect earlier

That's a rather good reason why these shouldn't be used as motivation behind design.

@henrikt-ma
Copy link
Collaborator

  • Revert adding this to Modelica 3.5 and investigate it further for a future release.

I have nothing agains the added clarification, so no need to revert all of it.

@HansOlsson
Copy link
Collaborator

So, let's do a poll then, I assume this is what you mean:

  • Keep the current addition. (rocket)

In the case of hierarchical components it is applied to all components, and if both of them have annotations then \lstinline!Evaluate = true! takes precedence.

  • Revert adding the second part making it: (eyes)

In the case of hierarchical components it is applied to all components.

@henrikt-ma
Copy link
Collaborator

Yes, that makes it clear what is being decided. Thanks!

@eshmoylova
Copy link
Member

Just to confirm, does the second option means that the annotation on a record, for example, overrides any possible annotations on the record elements? So the priority is given to the annotation on the record regardless of whether it is true or false?

@henrikt-ma
Copy link
Collaborator

Just to confirm, does the second option means that the annotation on a record, for example, overrides any possible annotations on the record elements? So the priority is given to the annotation on the record regardless of whether it is true or false?

Yes, this is what I consider just a clarification of the old formulation.

It has struck me, however, that a useful way of doing this that would be an alternative to (rocket), but without giving precedence to either Evaluate = true or Evaluate = false, would be to say that the Evaluate on the hierarchical component would only apply to children that don't already have Evaluate. I call this an alternative to (rocket) because it would also be an actual change rather than a clarification.

@HansOlsson
Copy link
Collaborator

Just to confirm, does the second option means that the annotation on a record, for example, overrides any possible annotations on the record elements? So the priority is given to the annotation on the record regardless of whether it is true or false?

Yes, this is what I consider just a clarification of the old formulation.

To me that isn't clear.

It has struck me, however, that a useful way of doing this that would be an alternative to (rocket), but without giving precedence to either Evaluate = true or Evaluate = false, would be to say that the Evaluate on the hierarchical component would only apply to children that don't already have Evaluate. I call this an alternative to (rocket) because it would also be an actual change rather than a clarification.

If we go without giving precedence to Evaluate=true, I would say that only applying it to components that don't already have Evaluate would be the cleanest.

@eshmoylova
Copy link
Member

OK, so it was worth clarifying :-) I agree with @HansOlsson that it would make sense to keep any annotation that was already there.

@henrikt-ma
Copy link
Collaborator

If we go without giving precedence to Evaluate=true, I would say that only applying it to components that don't already have Evaluate would be the cleanest.

This is also the option that would be the easiest to our poor users to understand: when you see Evaluate = x on a parameter, what you see is what you get. Put differently: Without this logic, proper support for Evaluate in a Modelica tool would require a rather fancy user interface to show what's going on when browsing a component hierarchy (some variant of showing effective modifications), so the user doesn't see Evaluate = x for a component that actually has been overridden by Evaluate = y in the current subtree of the component hierarchy.

@dietmarw
Copy link
Member Author

I was trying to poll but it is really hard to understand the short sentence that is polled on. Could you write down the options a bit more clearly that also specifies what "it" and "them" means.

@HansOlsson
Copy link
Collaborator

HansOlsson commented Dec 21, 2020

I was trying to poll but it is really hard to understand the short sentence that is polled on. Could you write down the options a bit more clearly that also specifies what "it" and "them" means.

The original idea was to not specify it - and only handle non-conflicting ones, but after more thinking I conclude that if nothing is specified the expected result is that it works similarly for records where overriding the value for a record overrides it for all elements https://specification.modelica.org/master/inheritance-modification-and-redeclaration.html#merging-of-modifications
Anything else seems to be excessive language-lawyering.

That means the new variants are:
Current addition (rocket):
In the case of hierarchical components it is applied to all components, and if both of them have annotations then \lstinline!Evaluate = true! takes precedence.

Full text:

The annotation \lstinline!Evaluate! can occur in the component declaration, its type declaration, or a base-class of the type-declaration.
In the case of multiple conflicting annotations it is handled similarly to modifiers (e.g., an \lstinline!Evaluate! annotation on the component declaration takes precedence).
In the case of hierarchical components it is applied to all components, and if both of them have annotations then \lstinline!Evaluate = true! takes precedence.
The annotation \lstinline!Evaluate! only has effect for a component declared with the prefix \lstinline!parameter!.

Revert Evaluate=true precedence and clarify the issue (eyes):
In the case of hierarchical components it is applied to all components overriding any Evaluate-setting for specific components.

Full text:

The annotation \lstinline!Evaluate! can occur in the component declaration, its type declaration, or a base-class of the type-declaration.
In the case of multiple conflicting annotations it is handled similarly to modifiers (e.g., an \lstinline!Evaluate! annotation on the component declaration takes precedence).
In the case of hierarchical components it is applied to all components, overriding any Evaluate-setting for specific components.
The annotation \lstinline!Evaluate! only has effect for a component declared with the prefix \lstinline!parameter!.

@henrikt-ma
Copy link
Collaborator

henrikt-ma commented Dec 21, 2020

The original idea was to not specify it - and only handle non-conflicting ones, but after more thinking I conclude that if nothing is specified the expected result is that it works similarly for records where overriding the value for a record overrides it for all elements https://specification.modelica.org/master/inheritance-modification-and-redeclaration.html#merging-of-modifications
Anything else seems to be excessive language-lawyering.

This was also my clear opinion at the beginning of this discussion. While I still think that the analogy with modifying an entire record is a strong argument in favor of overriding for all components, I can also see that there are some good reasons for not overriding for any components, and it would have been interesting to see what kind of support this would get in the poll. Reasons:

  • No need for advanced tool support to help a user understand that the Evaluate seen on a parameter might not be the one in effect.
  • Setting Evaluate on hierarchical components gets tricky, because one never knows what important settings of Evaluate deeper down in the components that will be overridden.
  • Unlike modification, there is no way to only apply Evaluate to selected descendants of the current component declaration, so applying Evaluate to individual parameters down in the component class definitions will be the best approximation, and then one doesn't want these to be without effect due to Evaluate being set higher up in the component hierarchy.

HansOlsson added a commit to HansOlsson/ModelicaSpecification that referenced this issue Dec 23, 2020
@eshmoylova
Copy link
Member

I chose the eyes. My first thought way that I would have preferred keeping any existing annotations on subcomponents without overriding them. However, it is always possible to specify the annotation on every subcomponent individually when you define the class instead of on the record as a whole. And since it is impossible to specify annotations on subcomponents when you declare a hierarchical component, the only way to modify the annotation specified at the class declaration is by allowing the the annotation on the hierarchical component override the one on a subcomponent.

@HansOlsson
Copy link
Collaborator

Summing up numbers:
Rocket(1): HansOlsson
Eyes(3) - split in two: henrikt-ma, eshmoylova, qlambert-pro

Seems like a clear decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants