-
Notifications
You must be signed in to change notification settings - Fork 2
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
Dont assume that MSLP and theta ref pressure are the same #151
Conversation
How set in stone is this number in the test?
|
5282974
to
3e94218
Compare
Yeah, that was a bit of a kludge. We should really be testing this: https://github.com/CliMA/Thermodynamics.jl/blob/main/test/relations.jl#L837. I forget exactly why that wasn’t working. |
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.
This looks good. It's good to separate the different reference pressures playing different roles in thermodynamic functions.
Given that MSLP is only used as the reference pressure for entropies now, could you please rename this to p_ref_entropy
or something like it, to make clear what role it plays?
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.
My only concern is, is it possible that this opens the door for inconsistencies? Otherwise it seems good.
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.
Also, I agree that this is a breaking change, so will need to bump the minor version
@tapios - I changed the name in We use We use
|
3e94218
to
63d23f7
Compare
Of course, please do leave MSLP as a parameter if it's used for initial conditions etc. in ClimaAtmos. Please make the changes to |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
=======================================
Coverage 93.35% 93.35%
=======================================
Files 9 9
Lines 1114 1114
=======================================
Hits 1040 1040
Misses 74 74
☔ View full report in Codecov by Sentry. |
63d23f7
to
7056443
Compare
bors r+ |
Configuration problem: |
bors try |
tryConfiguration problem: |
Ah, right, we switched to the GH merge queue |
Let's try |
We were assuming that mean sea level pressure (MSLP) and the reference pressure used in potential temperature are the same. This is technically not a mistake. It is causing problems in ClimaAtmos where we were overwriting
MSLP=101325
value to be100000
hPa for some of our cases but not for others.I'm hoping that differentiating between the two would help us get rid of those overwrites
I kept the
MSLP
in temperature profiles and in thespecific_entropy_
reference pressure. Lucky for us entropy is not used in ClimaAtmos, so it doesn't really matter.