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 ReferenceAir.MoistAir #4116

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

@HansOlsson HansOlsson self-requested a review June 21, 2023 06:51
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.

Similar comment:
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.)

Replied in #4115 having multiple PR for the different examples isn't helpful.

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

Has been decided to merge all these PRs in meeting 2023-11-14. I suggested to merge all the related PRS.

@beutlich beutlich force-pushed the referenceair-moistair-unit-error branch from 3bcda8c to 0b4368f Compare November 14, 2023 17:30
@HansOlsson HansOlsson self-requested a review November 15, 2023 09:24
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.
The requirement to have two reviewers doesn't seem to be working well when we don't have enough domain-experts.

@henrikt-ma henrikt-ma added this to the MSL4.1.0 milestone Dec 12, 2023
@arunkumar-narasimhan arunkumar-narasimhan merged commit f586b42 into modelica:master Jan 14, 2024
2 checks passed
@henrikt-ma henrikt-ma deleted the referenceair-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
L: Media Issue addresses Modelica.Media
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants