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

Make propagation final of lifted parameters in Spring #4407

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

henrikt-ma
Copy link
Contributor

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
Copy link
Contributor Author

henrikt-ma commented May 23, 2024

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.

@beutlich beutlich removed their request for review May 24, 2024 05:09
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is ok, but I don't know if part of preferred style of Modelica.Mechanics - so I leave the decision to relevant library officers.

Copy link
Contributor

@christiankral christiankral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, This makes sense.

Copy link
Contributor

@tobolar tobolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that I Joe Engineer would never come to the idea c is final due to a clear initialization.
@MartinOtter could you review here?

@casella casella requested a review from MartinOtter June 22, 2024 16:09
@henrikt-ma
Copy link
Contributor Author

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?

@AHaumer AHaumer self-requested a review January 15, 2025 10:33
Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me

@tobolar I don't understand your comment.
c is a simple parameter that is propagated. It is not a good idea to let the user change Spring.spring.c to a different value than Spring.c. What do you mean by "initialization"?

@tobolar
Copy link
Contributor

tobolar commented Jan 15, 2025

@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?

@henrikt-ma
Copy link
Contributor Author

@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.

Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…ponent

Even though the modification of lineShapeType is not parameter lifting, it would make little sense to make it non-final when all other modifications are final.
@henrikt-ma
Copy link
Contributor Author

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 henrikt-ma requested a review from beutlich January 15, 2025 21:46
@henrikt-ma
Copy link
Contributor Author

henrikt-ma commented Jan 17, 2025

@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.

@beutlich
Copy link
Member

Well, three approvals and green CI checks ...

@beutlich beutlich changed the title Make propagation final of lifted parameter 'c' in Spring Make propagation final of lifted parameters in Spring Jan 17, 2025
@beutlich beutlich removed the request for review from MartinOtter January 17, 2025 16:10
@beutlich beutlich merged commit a5cf0f3 into modelica:master Jan 17, 2025
11 checks passed
@beutlich beutlich added this to the MSL4.2.0 milestone Jan 17, 2025
@beutlich beutlich added the L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody label Jan 17, 2025
@henrikt-ma henrikt-ma deleted the final-propagation-c branch January 18, 2025 22:11
@casella
Copy link
Contributor

casella commented Jan 21, 2025

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

@casella
Copy link
Contributor

casella commented Jan 21, 2025

@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.

maltelenz pushed a commit to maltelenz/ModelicaStandardLibrary that referenced this pull request Jan 21, 2025
* Make propagation final of lifted parameter 'c' in Spring

Fixes modelica#3707.

* Make propagation final of lifted parameter 's_rel0' in Spring

* Make propagation final of lifted parameters in Spring's lineForce component

Even though the modification of lineShapeType is not parameter lifting, it would make little sense to make it non-final when all other modifications are final.

* Make propagation of lifted parameter 'd' final in ModelicaTest example SpringDamperParallel

---------

Co-authored-by: Anton Haumer <[email protected]>
(cherry picked from commit a5cf0f3)
@maltelenz maltelenz modified the milestones: MSL4.2.0, MSL4.1.0 Jan 21, 2025
@beutlich
Copy link
Member

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

Done by #4520.

@beutlich beutlich modified the milestones: MSL4.1.0, maintenance Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No propagation of fixed = false in InitSpringConstant
8 participants