On branch jm-pr-multiple-instances-of-ccpp-physics #1000
Conversation
Changes to add an instance index to CCPP physics Changes to be committed: modified: physics/GFS_phys_time_vary.fv3.F90 modified: physics/GFS_phys_time_vary.fv3.meta modified: physics/h2ointerp.f90 modified: physics/mp_thompson.F90 modified: physics/mp_thompson.meta modified: physics/ozinterp.f90
Bump up dimension of is_initialized array to match other instances and add comment Changes to be committed: modified: physics/mp_thompson.F90
michalakes
requested review from
grantfirl,
ChunxiZhang-NOAA,
dustinswales,
gthompsnWRF and
RuiyuSun
as code owners
| @@ -48,7 +48,7 @@ module GFS_phys_time_vary | |||
|
|
|||
| public GFS_phys_time_vary_init, GFS_phys_time_vary_timestep_init, GFS_phys_time_vary_timestep_finalize, GFS_phys_time_vary_finalize | |||
|
|
|||
| logical :: is_initialized = .false. | |||
| logical, dimension(200) :: is_initialized = .false. ! why 200? | |||
There was a problem hiding this comment.
Maybe another option would be to define "is_initialized" as an interstitial variable? Then each instance would have its own copy. This also keeps the instance business out of the physics schemes, but does come with the burden of adding more interstitials. Either way, changes to the schemes are needed, but we may need to discuss this more.
There was a problem hiding this comment.
I think we could leave this as is. The logic to have is_initialized at the scheme-module level is already there, all that this is doing is make it a vector. But if there's a good reason for doing it differently (because similar changes were made to other schemes since this PR was created years ago), then that's fine too.
There was a problem hiding this comment.
@climbfuji I made this change for Thompson, see https://github.com/ufs-community/ccpp-physics/blob/ufs/dev/physics/MP/Thompson/mp_thompson.F90#L57. I prefer the interstitial approach, which wouldn't be limited to an arbitrary limit on the number of instances (200).
There was a problem hiding this comment.
Ok, that's fine with me. Do we want to finish this off and make the same change for the other scheme that needs it?
| @@ -54,7 +54,12 @@ subroutine read_h2odata (h2o_phys, me, master) | |||
| !--- h2o_pres - vertical pressure level (mb) | |||
| !--- h2o_time - time coordinate (days) | |||
| !--- | |||
| allocate (h2o_lat(latsh2o), h2o_pres(levh2o),h2o_time(timeh2o+1)) | |||
| ! NOTE: If there are multiple instances of CCPP physics, only the first instance | |||
There was a problem hiding this comment.
I'm surprised that this works. Only one instance is allocating space, but all instances are reading and writing to the same memory?
|
@dustinswales @grantfirl We'll need to revive this PR and make sure it finds its way into the authoritative codebase. I'll work on updating it to the latest NCAR main code (a few conflicts to resolve). |
|
@dustinswales @grantfirl I am going to revive this PR. How do you want me to proceed, since these changes are for NCAR main? Should I simply update the PR from NCAR main and resolve the conflicts? Are you going to bring this down to the ufs fork for testing? Thanks ... |
|
@dustinswales I thought these changes were already incorporated into ufs/dev? Or was it just partial? |
OK, it looks like ufs-community#75 and the followup ufs-community#150 partially contained some of these changes, at least for ozone physics. Since those were targeted at ufs-community, I'm thinking that it might be best to rebase this off of ufs/dev and target a PR there. |
|
@grantfirl Correct. Only the ozone physics were updated to address Johns issues. |
|
Sounds great, thank you both. I can help with the remaining changes after Dustin is done with h2o. |
|
@dustinswales and @grantfirl any updates on the multiple instance changes from @michalakes ? As I said above, I can help with the remaining changes after Dustin is done with H2O. Thanks! |
|
@climbfuji After ufs/dev #223, all that remains is to create a new interstitial for |
Thanks Dustin. I had some thoughts about this. I don't think you can use an interstitial variable for this. It ought to be a scheme module variable, otherwise it needs to be allocated by the host and saved there. Interstitials don't have any memory whatsoever. |
|
@climbfuji Sorry, I meant a host interstitial that persists (GFS_control_type). |
Ok, that makes sense. Different terminology ;-) |
|
@climbfuji Sorry, I have this (bad) habit of calling everything that comes into the physics an Interstitial, even though Interstitials are a diverse group. |
|
Can we push this across the finish line, please? |
Changes to be committed:
modified: physics/GFS_phys_time_vary.fv3.F90
modified: physics/GFS_phys_time_vary.fv3.meta
modified: physics/h2ointerp.f90
modified: physics/mp_thompson.F90
modified: physics/mp_thompson.meta
modified: physics/ozinterp.f90