Skip to content

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
@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