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

Delete variables dens and R_air from Modelica.Mechanics.MultiBody.Examples.Loops.Utilities.GasForce2 #4081

Merged
merged 2 commits into from
May 17, 2023
Merged

Delete variables dens and R_air from Modelica.Mechanics.MultiBody.Examples.Loops.Utilities.GasForce2 #4081

merged 2 commits into from
May 17, 2023

Conversation

tobolar
Copy link
Contributor

@tobolar tobolar commented Mar 3, 2023

Close #4080

@tobolar tobolar added enhancement New feature or enhancement L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody L: Units Issue addresses Modelica.Units labels Mar 3, 2023
@tobolar tobolar added this to the MSL4.0.1 milestone Mar 3, 2023
@tobolar tobolar requested a review from MartinOtter March 3, 2023 13:25
@beutlich beutlich modified the milestones: MSL4.0.1, MSL4.1.0 Mar 7, 2023
@qlambert-pro
Copy link
Contributor

@casella and @HansOlsson could also review that PR.

@TManikantan
Copy link
Contributor

@MartinOtter @casella @HansOlsson kindly look into the fix for this ticket.

HansOlsson
HansOlsson reviewed Apr 5, 2023
View reviewed changes
Modelica/Mechanics/MultiBody/Examples/Loops/Utilities/GasForce2.mo Outdated
@@ -24,7 +24,8 @@ model GasForce2 "Rough approximation of gas force in a combustion engine's cylin
SI.Velocity v_rel "Relative piston velocity (<0: compression; >0: expansion)";

protected
constant SI.SpecificHeatCapacity R_air = Modelica.Constants.R/0.0289651159;
constant SI.MolarMass MM_gas = 0.0289651159;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constant SI.MolarMass MM_gas = 0.0289651159;
constant SI.MolarMass MM_gas = 0.0289651159 "Molar mass for dry air; should be changed";

This is somewhat complicated. Just having the value for "air" and calling it "gas" without any explanation seems confusing to me.
I see the following possibilities:

  • Explain it as proposed.
  • Call it "air" as before - but then we get the question of why there's just air in a combustion chamber.
  • Call it "gas" without explanation - but using the name "gas" for "air" seems as if we know it is bad, and just try to hide it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HansOlsson I fully agree with you. To rename to "gas" was just to be complementary with "gasForce".
To explain the model as purposed, we need the proper molar mass of the gas. This need some recherche. And, of course, the simulation results will change then.
But I agree this is the way we should preferre.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reviewed the model and, actually, R_air / R_gas is used just to calculate the gas' density dens which itself is not further used. So I tend to remove the variables R_air and dens completely. The regression results shall not be concerned.

Copy link
Member

Choose a reason for hiding this comment

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

I reviewed the model and, actually, R_air / R_gas is used just to calculate the gas' density dens which itself is not further used. So I tend to remove the variables R_air and dens completely. The regression results shall not be concerned.

Yes, this makes sense: Remove R_air and dens.

@TManikantan
Copy link
Contributor

@tobolar were you able to come up with something considering the suggestions of @HansOlsson.Kindly commit the fix if you have worked it out. Many Thanks

@TManikantan
Copy link
Contributor

@tobolar were you able to work through the problem?Kindly commit if you have found the fix.Many Thanks..

@tobolar
Copy link
Contributor Author

tobolar commented May 9, 2023

were you able to work through the problem?

See my last comment #4081 (comment)
I would appreciate if @HansOlsson and @MartinOtter could comment as well.

@HansOlsson
Copy link
Contributor

were you able to work through the problem?

See my last comment #4081 (comment) I would appreciate if @HansOlsson and @MartinOtter could comment as well.

I agree with that idea - but I guess the thumb-up was a bit too subtle.

@TManikantan
Copy link
Contributor

were you able to work through the problem?

See my last comment #4081 (comment) I would appreciate if @HansOlsson and @MartinOtter could comment as well.

@tobolar sorry i somehow missed your comment

@tobolar
Copy link
Contributor Author

tobolar commented May 16, 2023

the thumb-up was a bit too subtle

@HansOlsson Sorry, I really overlooked it. :-O

@tobolar
Copy link
Contributor Author

tobolar commented May 16, 2023

Both dens and R_airdeleted now. And removed dens from comparison signals of ModelicaTest.

@TManikantan
Copy link
Contributor

@MartinOtter @HansOlsson would you please approve the PR as the comments are addressed?

HansOlsson
HansOlsson approved these changes May 16, 2023
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.

@MartinOtter MartinOtter removed the request for review from casella May 17, 2023 15:26
@MartinOtter MartinOtter merged commit 2c7116c into modelica:master May 17, 2023
@tobolar tobolar deleted the issue4080_gasForce2 branch May 17, 2023 15:40
@beutlich beutlich added example Issue only addresses example(s) and removed enhancement New feature or enhancement L: Units Issue addresses Modelica.Units labels Jun 1, 2023
@beutlich beutlich changed the title Introduce molar mass for better balance of SI units Delete variables dens and R_air from Modelica.Mechanics.MultiBody.Examples.Loops.Utilities.GasForce2 Jun 1, 2023
@maltelenz maltelenz mentioned this pull request Mar 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinOtter MartinOtter MartinOtter approved these changes

@HansOlsson HansOlsson HansOlsson approved these changes

Assignees

@tobolar tobolar

Labels
example Issue only addresses example(s) L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody
Projects
None yet
Milestone
MSL4.1.0
Development

Successfully merging this pull request may close these issues.

Unit error in Modelica.Mechanics.MultiBody.Examples.Loops.Utilities.GasForce2
6 participants
@tobolar @qlambert-pro @TManikantan @HansOlsson @MartinOtter @beutlich