Back-port of #3291 for the pump model into MSL 3.2.3 maintenance

[casella]

This PR back-ports into MSL 3.2.3 maintenance some of the changes that were made to MSL 4.0.0 in #3291, to make sure that the PumpingSystem example can be reliably initialized.

A homotopy expression and a parameter checkValveClosedInit was introduced used to manage the CheckValve=true case in the pump model. The code of the pump model is now exactly the same as in MSL 4.0.0 (check here and here), which is the combined result of applying 1d9e75e followed by 94fea03.

The new parameter checkValveClosedInit defaults to noHomotopy as suggested by @HansOlsson in #3291, hence the default behaviour is exactly the same as in released MSL 3.2.3, making the fix fully backwards compatible.

Then, in order to ensure the convergence in OpenModelica of the PumpingSystem example, which starts with the pump having the check valve closed, d72f405 was also applied, changing the set up of the homotopy parameter for this specific test case.

Checked with OpenModelica 1.16.0 and Dymola 2020x. The best performance is obtained when also #3599 is included.

[HansOlsson]

Looks good to me.

[beutlich]

Adding new components to (base) classes is - strictly seen - not legal for maintenance releases.

[casella]

@beutlich, I know. I'm inclined to make an exception here, since 3.2.3 is an end-of-line maintenance versions which will have no backwards compatible new releases, i.e. 3.3.0, where I can put in this change. If we want to keep 3.2.3 in the long term and keep fixing it, we have to make some compromise.

Adding homotopy here is essential to get robust intialization into 3.2.3 maintenance - the extra parameter is essential because there are different homotopies, depending on the expected initial condition, and I need to select one.

The default value corresponds to the old behaviour, so the change is 100% backwards compatible.

If you see any other solution to fix this issue, I'm fine with it. Otherwise I think it's better to have a more robust MSL 3.2.3 than strictly respecting formal rules in a borderline case as MSL 3.2.3 maintenance.

What do you think?

[beutlich]

It is a trade-off which I believe @MartinOtter should decide in the end.

[MartinOtter]

I think in this very special situation it is fine to make an exception to the formal rules and I agree with Francescos proposal.

[beutlich]

Usually we add such backward-compatibility breaking changes in the release notes.

[beutlich]

Please see the appropriate documentation (added by bf8b927 for MSL v3.1).

As a recommendation, a valid bug-fix to the maintenance branch may contain one or more of the following changes.

• [..]
• Introducing a new name in the public section of a class (model, package, ...) or in any section of a partial class is not allowed. Since otherwise, a user might use this new name and when storing its model and loading it with an older bug-fix version, an error would occur.

[casella]

I think the reference to class is unnecessarily too generic.

I agree this is the case if we add a new model to a package class. But I really can't see how this could be a problem if we add a new parameter to a model class. How could a user use this parameter name? The only possible cause of conflict could be if the user extended the model and added a new parameter to it, which happened to have the same name, but a different meaning. Of course we can't rule this out 100%, but I would say this is really unlikely.

Do I miss something? Should we perhaps update that documentation?

[beutlich]

The main reason is, that models saved in MSL v3.2.3 Build 5 will no longer be compatible with MSL v3.2.3 Build 4. Usually we claim this kind of compatibility.

[casella]

Aha. I always thought that being backwards compatible meant that I could use the new library with an old model, not the other way round.

But then this would also apply to minor releases. Aren't we claiming that they are backwards compatible, according to semver rules?

[beutlich]

The expectation is/was, that no new components are introduced between different builds of MSL v3.2.3 (or according to semver, between different patch releases of MSL v4.0.x).

[beutlich]

If you want to apply this change, the newly introduced parameter needs to be mentioned in the release notes in the following table:

ModelicaStandardLibrary/Modelica/package.mo

Lines 2459 to 2481 in 13e7949

	<p><br>
	The following <font color=\"blue\"><strong>existing components</strong></font> have been <font color=\"blue\"><strong>changed</strong></font> in a <font color=\"blue\"><strong>non-backward compatible</strong></font> way:
	</p>
	<table border=\"1\" cellspacing=0 cellpadding=2 style=\"border-collapse:collapse;\">
	<tr><td colspan=\"2\"><strong>Modelica.Blocks</strong></td></tr>
	<tr><td>Interfaces.PartialNoise<br>Noise.UniformNoise<br>Noise.NormalNoise<br>Noise.TruncatedNormalNoise<br>Noise.BandLimitedWhiteNoise</td>
	<td>As a side-effect of the corrected computation in Modelica.Math.Random.Utilities.impureRandomInteger the <code>localSeed</code> parameter is computed differently if <code>useAutomaticLocalSeed</code> is set to true.</td></tr>
	<tr><td colspan=\"2\"><strong>Modelica.ComplexBlocks.Routing</strong></td></tr>
	<tr><td>ComplexPassThrough</td>
	<td>Fixed the connector types from Real to Complex</td></tr>
	<tr><td colspan=\"2\"><strong>Modelica.Mechanics.MultiBody</strong></td></tr>
	<tr><td>World</td>
	<td>Added new parameter <code>animateGround</code> for optional ground plane visualization.<br>
	Users that have copied the World model (of MSL 3.0, 3.0.1, 3.1, 3.2, 3.2.1, or 3.2.2) as an own World model and used it as inner world component, might have broken their models.
	Generally, for MSL models with sub-typing (due to inner/outer), it is strongly suggested to extend from this MSL model, instead of copying it.</td></tr>
	<tr><td colspan=\"2\"><strong>Modelica.Media.Interfaces</strong></td></tr>
	<tr><td>PartialMedium</td>
	<td>Added new constant <code>C_default</code> as default value for the trace substances of medium.<br>
	Users that have created an own medium by inheritance from the PartialMedium package and already added the C_default constant, might have broken their models.<br>
	Users that have copied the PartialMedium package (of MSL 3.0, 3.0.1, 3.1, 3.2, 3.2.1, or 3.2.2) as an own Medium package, might have broken their models.
	Generally, for MSL classes with sub-typing (due to a replaceable declaration), it is strongly suggested to extend from this MSL class, instead of copying it.</td></tr>
	</table>

[MartinOtter]

Sorry, I am not good enough in github to understand the current status: Is the request from Thomas (update of release notes) included in the pull request?

"This branch is out-of-date with the base branch": Is it still fine to "merge pull request"?

[beutlich]

Sorry, I am not good enough in github to understand the current status: Is the request from Thomas (update of release notes) included in the pull request?

No, not yet.

"This branch is out-of-date with the base branch": Is it still fine to "merge pull request"?

I will care about it, i.e., rebase and force-push soon.

[arunkumar-narasimhan]

@HansOlsson @casella, this is quite an old PR that is open even after approvals. Did this slip out somehow from earlier maintenance release?

[HansOlsson]

@HansOlsson @casella, this is quite an old PR that is open even after approvals. Did this slip out somehow from earlier maintenance release?

I have no idea, but unless we are releasing a maintenance release of 3.2.3 it doesn't matter; so even if it could be merged it might be simpler to close without merging.

[beutlich]

If milestone MSL4.0.1 was closed (w/o releasing/tagging anything), then also MSL3.2.3+maint should be closed. Basically @casella dropped the maintenance promise we aligned some years ago.

@HansOlsson @casella, this is quite an old PR that is open even after approvals. Did this slip out somehow from earlier maintenance release?

Note that my previous review comment is still valid.