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

Update params #521

Closed
wants to merge 8 commits into from
Closed

Update params #521

wants to merge 8 commits into from

Conversation

syha
Copy link
Collaborator

@syha syha commented Jul 28, 2023

Description:

Parameters are updated in model_mod.f90 to be consistent with those in the MPAS-A model (Version 7+).

Fixes issue

fixes #251
theta_m and pressure use rvord (=1.608362) instead of 1.61.
cp is now updated from 1003.0 to 1004.5 and cv is changed from 716.0 to 717.5.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Documentation changes needed?

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Tests

In 2-member tests with a single obs for sounding temperature at 700 hPa, new parameters changed all the analysis variables.

Checklist for merging

  • Updated changelog entry
  • Documentation updated
  • Update conf.py

Checklist for release

  • Merge into main
  • Create release from the main branch with appropriate tag
  • Delete feature-branch

Testing Datasets

  • Dataset needed for testing available upon request
  • Dataset download instructions included
  • No dataset needed

! redefined here for consistency with the model (MPAS/src/framework/mpas_constants.F).
real(r8), parameter :: rgas = 287.0_r8 ! = R_d (Gas constant for dry air [J kg-1 K-1])
real(r8), parameter :: rv = 461.6_r8 ! = R_v (Gas constant for water varpor [J kg-1 K-1])
real(r8), parameter :: cp = 7.*rgas/2. ! = 1004.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
real(r8), parameter :: cp = 7.*rgas/2. ! = 1004.5
real(r8), parameter :: cp = 7.0_r8*rgas/2.0_r8 ! = 1004.5

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better to me.

@hkershaw-brown hkershaw-brown added the mpas Model for Prediction Across Scales (MPAS) label Jul 28, 2023
@hkershaw-brown
Copy link
Member

MPAS-A model (Version 5+)
#429 (comment)

@@ -7380,7 +7381,7 @@ function theta_to_tk (ens_size, theta, rho, qv, istatus)

integer, intent(in) :: ens_size
real(r8), dimension(ens_size), intent(in) :: theta ! potential temperature [K]
real(r8), dimension(ens_size), intent(in) :: rho ! dry density
real(r8), dimension(ens_size), intent(in) :: rho ! dry air density [kg/m3]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a bunch of subroutine arguments labelled as dry density. Should these all be dry air density?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: it is one in compute_full_pressure

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Here, rho is all 'dry air density'. Nice catch!

@kdraeder
Copy link
Contributor

@soyoung closed #429 because the original pressure calculation appears to be correct,
but the commits to include a new qv^2 term seem to still be in this PR.
E.g. Bug fix for full pressure with the additional term for qv^2.

@nancycollins nancycollins mentioned this pull request Aug 2, 2023
12 tasks
@@ -7457,7 +7458,7 @@ subroutine compute_full_pressure(ens_size, theta, rho, qv, pressure, tk, istatus
tk = theta_to_tk(ens_size, theta, rho, qv_nonzero, istatus)

where (istatus == 0) ! We only take non-missing tk here
pressure = rho * rgas * tk * (1.0_r8 + 1.61_r8 * qv_nonzero)
pressure = rho * rgas * tk * (1.0_r8 + rvord * qv_nonzero)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wanted to leave a comment on line 7445 - the 'dry density' comment should be 'dry air density' to be consistent. i think that's the last place 'dry density' is in a comment (that i could find) that needs changing. if all these suggested changes are ok, should we go ahead and accept them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nancycollins I'm going to close this pull request and put the constant fix in cleanly.
As Kevin says, it is probably best not to have the Jedi equations in the commits, and we need to add documentation adding about the constant changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hkershaw-brown, just for clarification, there are no jedi equations or comments here at all. In this PR, I only updated the constants based on the model. I'm curious how you can make the fix cleaner?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the Jedi stuff is in the commit history:

Screen Shot 2023-08-02 at 4 31 25 PM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syha I'm aware you're busy with other things, so I'll put the fix in (I'll add you as a joint commit so you show up in the contributions). Thanks, Helen

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the commit history, not the code itself. Got it. Thanks!

@hkershaw-brown
Copy link
Member

closing - will redo without the Jedi commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mpas Model for Prediction Across Scales (MPAS)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Constants in the model_mod for MPAS
4 participants