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

Fix unit errors in Examples.MoistAir #4115

Merged

Conversation

henrikt-ma
Copy link
Contributor

No description provided.

@henrikt-ma henrikt-ma added the L: Media Issue addresses Modelica.Media label Apr 6, 2023
@qlambert-pro
Copy link
Contributor

This would help address the issues reported in #4099

@HansOlsson
Copy link
Contributor

I'm not sure if this is needed - and would discuss it together with #4097

@beutlich beutlich added the example Issue only addresses example(s) label Apr 17, 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.

Wait for #4103 to see if PressureRate gets introduced.

@HansOlsson HansOlsson self-requested a review June 21, 2023 06:44
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 examples in MSL should also be seen as examples of how people should write models.
The previous example worked for that - the new one does not.

Telling users that they cannot write such a simple test (as currently), but must introduce a number of new declaration that provides no benefit doesn't seem nice and will instead turn off users.

It also tells users that unit-checking is a chore.

If we wanted to give users something better we could do that, but then we should have public parameters, and sweep between two states with different pressures (p1A=1e5 at time=0 and p1B=2e5 at time=1) instead of a pressure slope, since that is more intuitive.

And the states should have pressure, temperature and mass-fractions for consistency reasons.

Done correctly that would automatically be unit-correct, and indicate how to sweep a medium in a parameterized way.

@henrikt-ma
Copy link
Contributor Author

Done correctly that would automatically be unit-correct, and indicate how to sweep a medium in a parameterized way.

Are you suggesting to modify the example this way as part of this PR? (I could do that. No problem.)

@HansOlsson
Copy link
Contributor

Done correctly that would automatically be unit-correct, and indicate how to sweep a medium in a parameterized way.

Are you suggesting to modify the example this way as part of this PR? (I could do that. No problem.)

I'm saying that the current PR is to me worse than the code in master branch - it's harder to understand, having lots of parameters with non-intuitive meaning, and just indicate to users that unit-checking is a chore that reduces readability.

I'm suggesting that it may be possible to get something that is easier to understand, general, and unit-correct; see #4103 and #4117 - and also that it is complicated to have discussion of seemingly identical issues spread over multiple PR.

@hubertus65 hubertus65 dismissed HansOlsson’s stale review November 14, 2023 14:44

Proposed changes agreed on for merging in MAP-LIB meeting 2023-11-14

@beutlich beutlich force-pushed the examples-moistair-unit-error branch from b774630 to 6e81a36 Compare November 14, 2023 17:29
@HansOlsson HansOlsson self-requested a review November 15, 2023 09:23
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, according to MAP-Lib.

@henrikt-ma henrikt-ma added this to the MSL4.1.0 milestone Dec 12, 2023
@arunkumar-narasimhan arunkumar-narasimhan merged commit 5d4a319 into modelica:master Jan 14, 2024
1 check passed
@henrikt-ma henrikt-ma deleted the examples-moistair-unit-error branch January 15, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example Issue only addresses example(s) L: Media Issue addresses Modelica.Media
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants