modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
452 stars 165 forks source link

MSL v4.1.0-beta.1 feedback: Missing conversion and convertMessage would help #4330

Open m-kessler opened 4 months ago

m-kessler commented 4 months ago

Dymola has a conversion script checker integrated, which reports that there is one missing conversion when coming from 4.0.0.

Run the command below with MSL 4.1.0 beta 1 loaded in Dymola 2024x:

DymolaCommands.SimulatorAPI.checkConversion(
  library="Modelica",
  from="4.0.0",
  oldVersions={"C:\Program Files\Dymola 2024x\Modelica\Library\Modelica 4.0.0\package.mo"})

This creates conversion_report.html in the Dymola working directory with following content:


Inconsistent conversion script auto, for version 4.0.0.
MessageClassComponent
Old component is missing, without conversionModelica.Electrical.PowerConverters.DCDC.Control.SignalPWMgreaterEqual

The component greaterEqual in the reported class has been renamed to greaterEqual_p. I am not sure if I would create a conversion script just for this single component. I simple workaround would be to rename the component back.

If you decide to create a conversion script, I would also include messages for the now protected mu0 in Mechanics.Rotational.Components.Brake and others . One of the models in the ElectrifiedPowertrains library currently uses this variable and a conversions message would have helped to quickly identify the problem.

beutlich commented 4 months ago

Thanks for the in-depth testing and reporting. Originally we did not aim for a conversion script for a minor release such as v4.1.0 (because following semantic versioning as promised by v4.0.0 we must not break things in minor versions).

  1. About Modelica.Electrical.PowerConverters.DCDC.Control.SignalPWM: This was changed in #3897 (where backwards-compatibilty was claimed in https://github.com/modelica/ModelicaStandardLibrary/pull/3897#issue-1052770320). I believe it's not simply done with renaming this component back.

  2. About Mechanics.Rotational.Components.Brake (and other mechanical components): We have been aware of the compatibility breakage (and thus the broken promise on semantic versioning) of #3662 and #3690 as we at least documented it already in the release notes in https://github.com/modelica/ModelicaStandardLibrary/blob/f9bddf86b19027bf2223b3e5a29647a35550e5d5/Modelica/package.mo#L2532-L2547

I see several options:

  1. Revert the breaking changes for v4.1.0.
  2. Revert the breaking changes and also introduce the new models under new names, SignalPWM2 or Brake2.
  3. Keep the breaking changes, mitigate them as much as possible, document them (where missing, i.e. SignalPWM) and also document the violation of previously given semantic versioning promise.
  4. Keep the breaking changes, add a conversion script and still name it v4.1.0.
  5. Keep the breaking changes, add a conversion script and also add the old models to ObsoleteModelicaX and add a test suite similar to ModelicaTestConversion4. Name it v5.0.0.

Note, that none of the options comes for free. Option 3 seems to be the most pragmatic one, but still might be a wrong decision if broken compatibility is accepted in a minor release. I am aware that semantic versioning might be controversial (especially in the Modelica eco system), but for sure, we must not claim compatibility and break it deliberately.

m-kessler commented 4 months ago

https://github.com/modelica/ModelicaStandardLibrary/pull/3897#issue-1052770320). I believe it's not simply done with renaming this component back.

Are you sure? With the default setup the behaviour of the block should not have changed. The new parameters refType and commonComparison have default values which make SignalPWM backward compatible.

Click for graphical comparison of SignalPWM ![SignalPWM](https://github.com/modelica/ModelicaStandardLibrary/assets/19187449/ae92fbc4-8027-4287-8046-6699a90068b4)
m-kessler commented 4 months ago

About Mechanics.Rotational.Components.Brake (and other mechanical components): We have been aware of the compatibility breakage ... as we at least documented it already in the release notes in

I think a (maybe otherwise empty) conversion script with detailed info would be better than just mentioning the change in the release notes. When I read the release notes, I did not remember that one of the thousands of classes we have in our libraries is using mu0.

Something like this would make sure that anyone who is using mu0 is informed:

convertMessage(
  "Mechanics.Rotational.Components.Brake",
  "Variable is now a protected parameter and should not be accessed any more form outside.",
  "mu0")
HansOlsson commented 4 months ago

About Mechanics.Rotational.Components.Brake (and other mechanical components): We have been aware of the compatibility breakage ... as we at least documented it already in the release notes in

I think a (maybe otherwise empty) conversion script with detailed info would be better than just mentioning the change in the release notes. When I read the release notes, I did not remember that one of the thousands of classes we have in our libraries is using mu0.

It's possible. However, since it still exists (even if protected) one can argue that it is not that problematic; as one will detect it afterwards. If it had been deleted or renamed the need would have been clearer.

HansOlsson commented 4 months ago

#3897 (comment)). I believe it's not simply done with renaming this component back.

Are you sure? With the default setup the behaviour of the block should not have changed.

Nice presentation of the difference - I agree that the greaterEqual-block does the same in both models.

beutlich commented 4 months ago

Are you sure?

Hm, not sure. I was only looking at the types: In v4.0.0 greaterEqual is of type Modelica.Blocks.Logical.Less whereas in v4.1.0-beta.1 greaterEqual_p is of type Modelica.Blocks.Logical.Greater (and not Modelica.Blocks.Logical.GreaterEqual as the name would suggest, which seems odd anyway). Yes, I noticed that signal inputs are switched between zeroOrderHold and sawtooth. Even if the name might be identical, the type still differs, which can also be considered breaking compatibility.

When I read the release notes, I did not remember that one of the thousands of classes we have in our libraries is using mu0.

I'd like to follow @tobolar's comment to unprotect mu0 again for a minor release: https://github.com/modelica/ModelicaStandardLibrary/pull/3662#issuecomment-732028326

m-kessler commented 4 months ago

I was only looking at the types: In v4.0.0 greaterEqual is of type Modelica.Blocks.Logical.Less whereas in v4.1.0-beta.1 greaterEqual_p is of type Modelica.Blocks.Logical.Greater (and not Modelica.Blocks.Logical.GreaterEqual as the name would suggest, which seems odd anyway).

Good point, I did not note that the type has changed. I guess it should be GreaterEqual then.

tobolar commented 4 months ago

One of the models in the ElectrifiedPowertrains library currently uses this variable and a conversions message would have helped to quickly identify the problem.

@m-kessler if mu0 is really used, IMO a conversion script would not really help as there is no public variable comparable to mu0.

m-kessler commented 4 months ago

Sure, that's why I suggested convertMessage.

tobolar commented 4 months ago

So I would prefer to define mu0 public again. A convert message can be added in future when upgrading MSL and defining mu0 protected again.

casella commented 2 weeks ago

@m-kessler I am a bit confused by this ticket. Originally, it claimed that there is a non-backwards compatible component name change in Modelica.Electrical.PowerConverters.DCDC.Control.SignalPWM. Later comments by @beutlich also mention Mechanics.Rotational.Components.Brake (and other mechanical components)(which ones, exactly?) for which there was a non-backwards compatible change which has nothing to do with conversion and convertMessage, and was already dealt with in the release notes.

Regarding the original issue, to me this is really not a big deal. As I understand, @AHaumer and @christiankral made the converter model more general by having two blocks greaterEqual_p and greaterEqual_n that can be driven independently, instead of relying on just one block greaterEqual. So, the point is not that a component was renamed, but rather that the whole implementation was changed, to make it more general. So, I guess just renaming that component back won't solve the problem, it could actually make it even more confusing.

On the other hand, I understand that

To me this is the typical situation that can be handled by a line in the release notes, so I'm definitely in favour of option 3. in @beutlich's comment. Introducing conversion scripts for such a marginal change seems to me completely unnecessary overkill. MSL 4.1.0 should have no conversion scripts, to me this is a strong requirement. This is a big nuisance in terms of project management, considering that everybody uses the MSL, so it should be used as sparingly as possible. Preferably not more than once every 10 years.

Regarding the mu0 issue, I understand it was already deemed a minor issue only worth of a line in the release notes. @tobolar if you think this is not an optimal solution, please open a separate ticket and explain how you would like this issue to be handled, and exactly on which components of the library. Thanks!

casella commented 2 weeks ago

Although I understand that for this kind of PWM generation models using Greater or GreaterEqual would give exactly the same results (the ZC functions change sign exactly at the same point in time), I also find it slighly odd that the two blocks are named greaterEqual_p and greaterEqual_n, but then they are of type Greater. No big deal, but a bit odd.

I would then suggest either to change their type, or to change their name, to avoid confusion. @AHaumer was there any reason in favour of Greater vs. GreaterEqual for the new implementation?

m-kessler commented 2 weeks ago

@casella: Indeed, the ticket is a bit confusing, as I started to mix in the Brake / mu0 issue in the original post. Sorry for that.

Nevertheless, the whole issue is not a big deal and I did not expect to create such a big discussion. Just go for solution 3, its a reasonable choice.

Regarding:

Later comments by beutlich also mention Mechanics.Rotational.Components.Brake ... for which there was a non-backwards compatible change which has nothing to do with conversion and convertMessage, and was already dealt with in the release notes.

The change could be communicated to all users which are actually using m0 with convertMessage (additional to mentioning it in the release notes), so its not completely unrelated to conversion. But still, no big deal.