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

Revert adding unit to pi due to regressions #4191

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

HansOlsson
Copy link
Contributor

@HansOlsson HansOlsson commented Sep 18, 2023

As indicated in #4046 it we need to revert part of #4155 since it is causing too many regressions for a minor release.

With proposed unit-checking in Dymola this reduces the warnings from 2301 to 1224 (Most - but not all are unit-related.)

Obviously many of them could be corrected, but I don't see that we have the resources to focus on that for this minor release. The first models found were CompareTransformers, SeriesResonance in Modelica.Electrical.Analog.Examples.

 since it is causing too many regressions for a minor release.

With proposed unit-checking in Dymola this reduces the warnings from 2301 to 1224 (Most - but not all are unit-related.)

Obviously many of them could be corrected, but I don't see that we have the resources to focus on that for this minor release.
The first models found were CompareTransformers, SeriesResonance in Modelica.Electrical.Analog.Examples.
@HansOlsson HansOlsson added the L: Constants Issue addresses Modelica.Constants label Sep 18, 2023
@HansOlsson HansOlsson changed the title As indicated in #4046 it we need to revert part of #4155 since it is … Revert adding unit to pi due to regressions Sep 18, 2023
@henrikt-ma
Copy link
Contributor

henrikt-ma commented Sep 18, 2023

I would expect there to be a small number of root causes, and that we would have time to correct these before the freeze at the start of the next year.

Edit: Based on the affected examples mentioned above, the fix would be a simple matter of introducing parameters with the proper units instead of the literals used in the binding equations involving pi. The newly introduced parameters can be made protected when there is a need to protect agains backwards incompatibility in the future, if Modelica one day would have a mechanism for attaching units to literals (for example, L = (0.1)["V/A"] / (2 * pi)). Alternatively, the newly introduced parameters could be made public, which would actually add value to the examples as it makes it more natural to change values after translation.

@HansOlsson
Copy link
Contributor Author

I would strongly recommend to merge this, and then prioritize investigating and fixing the issues together with other items.

@beutlich beutlich added the V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases) label Sep 18, 2023
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.

Shouldn't the units of e, pi and gamma be consistent? It seems arbitrarily otherwise.

@beutlich beutlich added this to the MSL4.1.0 milestone Sep 18, 2023
Copy link
Collaborator

@mestinso mestinso left a comment

Choose a reason for hiding this comment

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

I'm ok with it so long as the intent is to resolve the unit issues in the offending models with pi in the future.

@beutlich Regarding the other ones: if those ones aren't problematic today, seems ok to leave to me.

@HansOlsson Is it valuable to add a note/comment in the code that there is an intent to change the unit for pi to 1 in the future? This also might make it feel less arbitrary for somebody who is viewing the code.

@henrikt-ma
Copy link
Contributor

I suggest that we get started right away cleaning up the uses of pi, and then we can see how far we get before the planned freeze early next year.

@hubertus65 hubertus65 merged commit 7253677 into modelica:master Oct 17, 2023
@beutlich beutlich removed the request for review from MartinOtter October 17, 2023 17:24
@HansOlsson HansOlsson deleted the RemovePIunit branch March 26, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Constants Issue addresses Modelica.Constants V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants