Remove direct physcons module dependencies from CCPP-Physics - #381
Remove direct physcons module dependencies from CCPP-Physics#381scrasmussen wants to merge 14 commits into
Conversation
- fpkapx function called before gfuncphys initializes its runtime constants, manually porting function
|
Note to reviewers: my setup had "remove whitespace at the end of lines" automatically turned on, so by the end of this work, removing the whitespace-cleanup changes would have been near impossible. But if you go to "Files changed" -> [Settings Button] -> "Hide whitespace", you can remove those changes so it is easy to visualize the differences |
It's easy to hide whitespaces when reviewing, but the real issue of a greatly increased chance for merge conflicts persists. Is it possible to turn off this setting until we've had the chance to go in and remove all trailing whitespaces from all files in one standalone PR? |
Yeah, that totally makes sense! I've added it back in with a commit, I also have a ufs-dev-whitespace-cleanup branch which removes the whitespace from all Fortran and meta files (no physcon changes). I didn't mean to force the whitespace-cleanup on everyone, do you think now would be a good time to submit that whitespace-cleanup branch as a PR? |
I think that's a good idea. Ideally, we'd have the codee formatter or another Fortran linter in place in CI to make sure it stays this way after your whitespace cleanup PR, but we can probably add a duct-tape solution to run in CI ourselves for just this format check? |
|
@scrasmussen As a followup, we should move the entire physcons.F90/meta file to reside with the host models (UFSATM, CCPP-SCM, ...) |
| rovcp = rd*rcpd ) | ||
| c5ies=c3ies*(tmelt-c4ies)) | ||
|
|
||
| real(kind=kind_phys) :: rd = 1.0E30_kind_phys |
There was a problem hiding this comment.
What is the purpose of initializing these module-level constants before the init phase is called?
| & cnvw(:,:), cnvc(:,:), dt_mf(:,:) | ||
| ! | ||
| real(kind=kind_phys), intent(out) :: ud_mf(:,:) | ||
| real(kind=kind_phys), intent(out), optional :: ud_mf(:,:) |
There was a problem hiding this comment.
Why was ud_f changed to optional? It doesn't appear to be optional to me. Also, the metadata doesn't reflect this. I'm guessing that we should just revert this?
| use cires_ugwp_triggers | ||
| use ugwp_driver_v0 | ||
| use drag_suite, only: drag_suite_run, drag_suite_psl | ||
| use ugwp_common_v0, only: ugwp_common_v0_init |
There was a problem hiding this comment.
There may be some pushback RE: linking to the CIRES ugwp scheme from the unified ugwp scheme, although the boundaries are often blurry!
|
|
||
| logical :: tables_are_initialized = .false. ! initialize satuation tables | ||
|
|
||
| real(kind_phys) :: grav = 1.0E30 |
There was a problem hiding this comment.
Same question RE: initializing to 1000. If it is necessary to initialize them, would it make sense to use something a little less plausible, e.g. -999.9? Something that would definitely cause a crash if somehow the setting of the constants didn't get done in the init phase?
There was a problem hiding this comment.
Doh. I missed the extra 0. 10^30 is definitely unplausible enough! The question regarding the need to initialize here remains though.
| public meltfrz_inst | ||
| public fix_up_clouds_2M | ||
| PUBLIC CLOUD_PTR_STUBS | ||
| #ifdef NEMS_GSM |
There was a problem hiding this comment.
I'm curious about this NEMS_GSM preprocessor flag. I'm guessing that this harkens back to a decade+ ago when this scheme was first being put into the GSM version of GFS? I can't see anywhere in the ufs-weather-model where this is being set. Are you sure that it is being used?
| this%pstr(iLev) = pstr4(iLev) | ||
| this%pkstr(iLev) = fpkapx(this%pstr(iLev)*100.0) | ||
| ! following line equivalent to funcphys's fpkapx=(p/1.e5_krealfp)**con_rocp | ||
| this%pkstr(iLev) = (this%pstr(iLev)*1.0e-3_kind_phys)**con_rocp |
There was a problem hiding this comment.
What was the reasoning behind making this local instead of invoking the function? Its simplicity?
| errmsg = '' | ||
| errflg = 0 | ||
|
|
||
| amdw = con_amd/con_amw |
There was a problem hiding this comment.
I don't love this in the run phase. It's minor, but it certainly doesn't need to be redone millions of times during a run.
| errflg = 0 | ||
|
|
||
| ! Set atomic weights | ||
| amdw = con_amd/con_amw |
|
This is probably above-and-beyond, but did you happen to search for literal constant lurking in the code? E.g., if we see |
| real (kind=kind_phys), dimension(:), intent(inout) :: lai, rca | ||
| real (kind=kind_phys), dimension(:), intent(inout), optional :: & | ||
| & wet1 | ||
| real (kind=kind_phys), intent(inout), optional :: wet1(:) |
There was a problem hiding this comment.
Saving a newline character?
| slc(i,k) = slsoil(k) | ||
| enddo | ||
| wet1(i) = smsoil(1) / smcmax !Sarah Lu added 09/09/2010 (for GOCART) | ||
| if (present(wet1)) then |
There was a problem hiding this comment.
Good optional arg catch.
|
Really nice and thorough work, @scrasmussen! A couple of minor questions to answer, and I'll approve afterwards. |
Description of Changes:
Tests Conducted:
Dependencies/Related PRs:
Documentation:
No new documentation is needed, the removal of direct dependencies on the global physcons module falls in line with the already documented principles of the CCPP framework and physics.