Evaluate number of phases parameter

[henrikt-ma]

This PR is limited to Modelica.Electrical and Modelica.Magnetic. It does for these sub-libraries what #3113 does for Modelica.Mechanics, namely change some components declared parameter to be constant instead in order to make clear to both users and tools that they can and are expected to be evaluated during translation.

There is ongoing discussion in #3113 whether the right way to do this would be with Evaluate=true instead of constant, and the two PRs should be settled the same way.

Note connection to #1966, which is fixed by the first commit.

Fix #1966.

[christiankral]

I am afraid I do not understand this PR. Why should m be constant in thePolyphase package, which is intended to model an arbitrary number of phases. The case m = 3 is very common in power engineering, but Polyphase is definitely not limited to three phases.

[beutlich]

I am afraid I do not understand this PR.

You need to read #3113 to get the actual purpose of constants ad evaluated parameters.

@hendrikt-ma I recommend to wait with further similar change proposals as long as #3113 is not merged. And given the current reviews it does not look that promising.

[henrikt-ma]

@hendrikt-ma I recommend to wait with further similar change proposals as long as #3113 is not merged. And given the current reviews it does not look that promising.

No problem. I don't have any more PRs like this in the pipe. I've done what I can to improve the situation, and now it's time to enjoy the show.

[henrikt-ma]

I am afraid I do not understand this PR. Why should m be constant in thePolyphase package, which is intended to model an arbitrary number of phases. The case m = 3 is very common in power engineering, but Polyphase is definitely not limited to three phases.

The key change here, is that the phase index is now a constant (fixing #1966). The constants were given a Dialog annotation so that tools should still let the user select the value similar to when they were parameters, but I didn't foresee that Dymola would hide the constants despite the presence of Dialog.

Then, with the phase index being a constant, it follows naturally that also the number of phases is also a constant.

In the end, I guess the lack of respect for Dialog in some tools and the unfinished status of modelica/ModelicaSpecification#2211 will force me to degrade these changes to use Evaluate=true instead…

[AHaumer]

I dislike the idea to use a constant with annotation(Dialog) instead of a parameter.

[AHaumer]

I dislike the idea to use a constant with annotation(Dialog) instead of a parameter.

[henrikt-ma]

I dislike the idea to use a constant with annotation(Dialog) instead of a parameter.

Should that be interpreted as preferring the use of Evaluate=true, or do you expect tools to do some other magic to end up with the expected equation systems?

Out of curiosity, is it because of the lack of Dymola support for constants with Dialog that you prefer a parameter, or do you see other reasons?

[AHaumer]

@henrikt-ma
In my understanding the number of phases is a parameter, not a constant. Evaluate=true is fine.

[henrikt-ma]

OK, so it's the confusion about the name constant, not the Modelica variability constant again… I really wish there was a name for constant that didn't come with so many expectations that don't have anything to do with variability. Anyway, the conclusion remains: not enough interest in using constant, and I'll have to litter the code with Evaluate=true instead. I'll get to actually doing it some day… :)

[claassistantio]

All committers have signed the CLA.

[dietmarw]

@henrikt-ma Is it fine for you to close this one?

[henrikt-ma]

Please don't close, as I'm planning to do this with the Evaluate=true fallback instead.

[henrikt-ma]

@AHaumer, per your request, I have now reverted the changes from parameter to constant, and replaced them with the use of Evaluate=true.

Although hidden inside an annotation, this still gives the user a chance to see that the intention is that these parameters should be evaluated during translation.

As long as new components are created by means of copying an existing component and modifying the copy, there's a chance that the Evaluate=true will also be present on similar parameters in the future. Otherwise, we will likely end up in an inconsistent state where the presence of Evaluate=true on a parameter that should be evaluated during translation is more or less random. However, compared to the other drawbacks of using Evaluate=true, I consider this a minor issue. (Note that it would be much more unlikely to get into similar randomness when using constant, as the Modelica tool will enforce the rules of variability.)

[beutlich]

LGTM.

[christiankral]

Just a side comment: This PR actually also reveals that we have two different implementation of the min = 3 attribute for the parameter m:

1. parameter Integer m(min=3) = 3 "Number of phases" annotation(Evaluate=true);
2. parameter Integer m(final min=1) = 3 "Number of phases" annotation(Evaluate=true);

@AHaumer I guess we should replace case 1. by 2. one day, shouldn't we?

[christiankral]

I am OK with this PR but I would like to have @AHaumer's explicit review before merging.

[beutlich]

I rebased and resolved the merge conflict.

@AHaumer Can you please add your review as @christiankral requested. Thank you.

[AHaumer]

Just a side comment: This PR actually also reveals that we have two different implementation of the min = 3 attribute for the parameter m:

1. `parameter Integer m(min=3) = 3 "Number of phases" annotation(Evaluate=true);`

2. `parameter Integer m(final min=1) = 3 "Number of phases" annotation(Evaluate=true);`

@AHaumer I guess we should replace case 1. by 2. one day, shouldn't we?

Depends where these 2 lines are found.
For Electrical.Machines min=3 is neccesary at the moment.
For PolyPhase min=1 is possible.

[beutlich]

I resolved the pseudo-merge conflicts again.

Resolved.