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.

Sign up for GitHub

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

Jump to bottom

Fix usage of Modelica.SIunits.Height in Modelica.Fluid #2834

Merged
merged 3 commits into from
Apr 4, 2019
Merged

Fix usage of Modelica.SIunits.Height in Modelica.Fluid #2834

merged 3 commits into from
Apr 4, 2019

Conversation

beutlich
Copy link
Member

This is an alternative proposal to #2639 taking all comments in account and keeping Modelica.SIunits untouched.

@beutlich beutlich added the L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation) label Feb 18, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone Feb 18, 2019
HansOlsson
HansOlsson previously approved these changes Feb 19, 2019
View reviewed changes
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.

Looks good.

@beutlich
refs modelica#2639: Fix usage of SIunits.Height
b87b2f5 
Forgot to commit previously.
@beutlich beutlich assigned beutlich and unassigned casella Mar 2, 2019
@beutlich
Copy link
Member Author

beutlich commented Mar 2, 2019

@casella OK for you?

@beutlich beutlich mentioned this pull request Mar 19, 2019
Closed
HansOlsson
HansOlsson previously approved these changes Mar 20, 2019
View reviewed changes
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.

Ok.

@beutlich
Copy link
Member Author

@casella @MartinOtter Friendly reminder to review.

@beutlich beutlich mentioned this pull request Mar 22, 2019
Closed
@casella
Further fixes to PartialPump
5ec9d29 
Changed Length into Position (more appropriate), and improved the simplified expression to use an approximation of the tangent to the pump curve around the initialization point
casella
casella approved these changes Mar 27, 2019
View reviewed changes
Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

OK for me. I added some more changes along the lines of what I wrote in #2370

@beutlich, would you mind creating a 3.2.3 maintenance branch and pushing this there also?

@beutlich
Copy link
Member Author

beutlich commented Mar 27, 2019 •
edited

would you mind creating a 3.2.3 maintenance branch and pushing this there also?

We have that branch already (maint/3.2.3). If it is considered as a bug, I will cherry-pick it and open a new pull request for maint/3.2.3 (once it is merged here).

@beutlich
Copy link
Member Author

@MartinOtter Since you were involved in the previous discussion of #2639, it would be very helpful, if you could review this pull request also. Thanks.

@beutlich beutlich added the bug Critical/severe issue label Mar 27, 2019
@beutlich
Copy link
Member Author

I confirm that SI.Position is used consistently after 5ec9d29.

@HansOlsson Can you please update your review.

HansOlsson
HansOlsson approved these changes Mar 29, 2019
View reviewed changes
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.

Looks good.
At first I was a bit concerned that dp_grav_a used Modelica.SIunits.Pressure instead of Modelica.SIunits.PressureDifference; but PressureDifference is just an alias to Pressure so it seems ok.

@beutlich
Copy link
Member Author

beutlich commented Mar 29, 2019 •
edited

Yes, I wanted to use PressureDifference at first, then I noticed that it is not used consistently at all in the MSL. Should probably go to a separate pull request.

@beutlich beutlich removed the request for review from MartinOtter April 4, 2019 15:58
@beutlich beutlich merged commit 68f9420 into modelica:master Apr 4, 2019
@beutlich beutlich deleted the fix-height branch April 4, 2019 15:59
@beutlich beutlich mentioned this pull request Apr 4, 2019
Merged
@beutlich
Copy link
Member Author

beutlich commented Apr 4, 2019

would you mind creating a 3.2.3 maintenance branch and pushing this there also?

We have that branch already (maint/3.2.3). If it is considered as a bug, I will cherry-pick it and open a new pull request for maint/3.2.3 (once it is merged here).

Done by #2876.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casella casella casella approved these changes

@HansOlsson HansOlsson HansOlsson approved these changes

Assignees

@beutlich beutlich

Labels
bug Critical/severe issue L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation)
Projects
None yet
Milestone
MSL4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@beutlich @casella @HansOlsson