Skip to content

Conversation

@qlambert-pro
Copy link
Contributor

This PR is meant to address some of the issues reported in #4099. If the opinions are positive I can prepare a larger patch set fixing them all.

@maltelenz maltelenz requested review from HansOlsson and casella March 29, 2023 08:46
@HansOlsson
Copy link
Contributor

I believe the important part for that test-case isn't unit-checking, but some explanation for the Example itself.

Here we have two "volumes" one where we have a constant enthalpy inflow and another where we sweep the pressure.

And then we mix their states around 30+/-10 s when simulating from 0 to 100s.

plot({"medium2.T","medium.T","state.T"}, colors={{28,108,200},{238,46,47},{0,140,72}});
plot({"medium2.p","medium.p","state.p"}, colors={{28,108,200},{238,46,47},{0,140,72}});

MoistAir

At least that's what I can see from the model, but it still doesn't answer: Why?

As for the model after the proposed changes: Note that the other examples in that package often run from 0 to 1s instead of 100s as in this example; so a parameter for pressure-rate is not that understandable on its own. On other hand there is an EnthalpyFlowRate for the other volume, and the change of pressure wasn't easier to understand in the original model.

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.

As indicated before I think the major issue is understanding the example, and the change doesn't help. It's also an unresolved question exactly how forgiving we should be for test-cases like this.

The following would make it more understandable IMO:

  • Explain that we sweep pressure to show something (I'm no fluid expert!)
  • We specifically sweep the pressure from 0 to 10bar for 100s; that seems more understandable than pressureRate so we might as well use that parametrization and thus avoid a new type.
  • We might as well have it as parameter instead of constant.

Co-authored-by: Henrik Tidefelt <[email protected]>
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, since Hubertus likes it. (Note that I haven't requested changes before on this one.)

@henrikt-ma henrikt-ma added this to the MSL4.1.0 milestone Dec 12, 2023
@arunkumar-narasimhan arunkumar-narasimhan merged commit 7f46072 into modelica:master Jan 14, 2024
@beutlich beutlich removed the request for review from casella January 14, 2024 19:30
@beutlich beutlich added L: Media Issue addresses Modelica.Media L: Units Issue addresses Modelica.Units labels Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L: Media Issue addresses Modelica.Media L: Units Issue addresses Modelica.Units

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants