ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

Make propagation final of lifted parameter 'c' in Spring

Open henrikt-ma opened this issue 1 year ago • 1 comments

Fixes #3707.

As there wasn't any arguments agains making the propagation of c final in #3707, I suggest that we go with this solution.

One could argue that there are more parameters whose propagation should be final, but I'm starting with this one which is sufficient to fix the issue.

henrikt-ma avatar May 23 '24 21:05 henrikt-ma

By the way, the discussion in MO-2748 didn't take us much further. It gives us a language to express initialization problems more clearly in Base Modelica, but doesn't provide a full specification of how full Modelica initialization should be turned into Base Modelica.

henrikt-ma avatar May 23 '24 21:05 henrikt-ma

It seems like we are not getting the attention of @MartinOtter on this one. @casella or @AHaumer, may I kindly ask for your help to step in and drive this forward?

henrikt-ma avatar Jan 15 '25 08:01 henrikt-ma

@AHaumer The reason for c final is #3707 (i.e. a clear model initialization). But this decision is not clear to a common user, so one could wonder why is c final but e.g. s_rel0 not?

tobolar avatar Jan 15 '25 15:01 tobolar

@AHaumer The reason for c final is #3707 (i.e. a clear model initialization). But this decision is not clear to a common user, so one could wonder why is c final but e.g. s_rel0 not?

Indeed, the common user would be right to wonder. If also the modification of s_rel0 was final, I suppose one should be able to make a variant of InitSpringConstant where s_rel0 instead of c is determined at initialization.

If that helps, I'd gladly also add final for s_rel0.

henrikt-ma avatar Jan 15 '25 18:01 henrikt-ma

What about the other lifted parameters of the lineForce component? Seems arbitrary to me to have final modifications at some place but not at other. I asked the same question in #2307 with @MartinOtter's reply to go for final parameter modifications consistently.

Done.

Also addressed a similar issue in ModelicaTest.MultiBody.Forces.SpringDamperParallel. I suggest that we avoid to let the scope of this PR creep to the bigger task of adding final for propagated parameters in general; the change for SpringDamperParallel is included because it addresses an actual issue of the same kind as reported in #3707.

Once this PR is merged, it can serve as a starting point that other PR's can refer to as motivation for doing similar cleanup in other models.

henrikt-ma avatar Jan 15 '25 21:01 henrikt-ma

@AHaumer The reason for c final is #3707 (i.e. a clear model initialization). But this decision is not clear to a common user, so one could wonder why is c final but e.g. s_rel0 not?

After addressing @beutlich's request for change, this is no longer the case.

henrikt-ma avatar Jan 17 '25 06:01 henrikt-ma

Well, three approvals and green CI checks ...

beutlich avatar Jan 17 '25 16:01 beutlich

@maltelenz, please back-port to maint/4.1.x

casella avatar Jan 21 '25 10:01 casella

@Esther-Devakirubai this should be mentioned in the release notes. In general it's not going to be a problem, unless for some reason somebody changes the low-level parameters (that are now final) instead of the propagated parameters at the higher level.

casella avatar Jan 21 '25 10:01 casella

@maltelenz, please back-port to maint/4.1.x

Done by #4520.

beutlich avatar Jan 21 '25 16:01 beutlich

@Esther-Devakirubai please mention this PR in the release notes, as technically non-backwards compatible. Thanks!

casella avatar Feb 11 '25 11:02 casella