-
Notifications
You must be signed in to change notification settings - Fork 169
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
Fix unit errors in ReferenceAir.MoistAir #4116
Conversation
This would help address the issues reported in #4099 |
I'm not sure if this is needed - and would discuss it together with #4097 |
There was a problem hiding this 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.
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. |
Has been decided to merge all these PRs in meeting 2023-11-14. I suggested to merge all the related PRS.
3bcda8c
to
0b4368f
Compare
There was a problem hiding this 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.
No description provided.