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.

Sign up for GitHub

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

Jump to bottom

On branch jm-pr-multiple-instances-of-ccpp-physics #1000

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

On branch jm-pr-multiple-instances-of-ccpp-physics #1000

wants to merge 2 commits into from

Conversation

michalakes
Copy link
Contributor 

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

michalakes added 2 commits February 28, 2023 14:41
On branch jm-pr-multiple-instances-of-ccpp-physics
71a5bae 
	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
On branch jm-pr-multiple-instances-of-ccpp-physics
d90e5fe 
	Bump up dimension of is_initialized array to match other instances and add comment
 Changes to be committed:
	modified:   physics/mp_thompson.F90
@michalakes michalakes marked this pull request as ready for review April 29, 2023 12:07
dustinswales
dustinswales reviewed May 2, 2023
View reviewed changes
physics/GFS_phys_time_vary.fv3.F90
@@ -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?
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

dustinswales
dustinswales reviewed May 2, 2023
View reviewed changes
physics/h2ointerp.f90
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised that this works. Only one instance is allocating space, but all instances are reading and writing to the same memory?

@climbfuji
Copy link
Collaborator

@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).

@climbfuji
Copy link
Collaborator

@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 ...

@grantfirl
Copy link
Collaborator

@dustinswales I thought these changes were already incorporated into ufs/dev? Or was it just partial?

@grantfirl
Copy link
Collaborator

@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.

@dustinswales
Copy link
Collaborator

@grantfirl Correct. Only the ozone physics were updated to address Johns issues.
To wrap this up we should to refactor the h2o photochemistry scheme identically to the ozone. Then the changes to mp_thompson and the GFS interstitials are trivial, we just need to move some things from module memory to interstitials OR define them with the instance dimension (I vote for the former).
If this sounds good I can do the h2o changes, but probably not for a week at least.

@climbfuji
Copy link
Collaborator

Sounds great, thank you both. I can help with the remaining changes after Dustin is done with h2o.

@climbfuji
Copy link
Collaborator

@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!

@dustinswales
Copy link
Collaborator

@climbfuji After ufs/dev #223, all that remains is to create a new interstitial for is_initialized in GFS_phys_time_vary.fv3.F90.

@climbfuji
Copy link
Collaborator

@climbfuji After ufs/dev #223, all that remains is to create a new interstitial for is_initialized in GFS_phys_time_vary.fv3.F90.

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.

@dustinswales
Copy link
Collaborator

@climbfuji Sorry, I meant a host interstitial that persists (GFS_control_type).
That's where I put all of the module variables that were causing problems with multiples instances.

@climbfuji
Copy link
Collaborator

@climbfuji Sorry, I meant a host interstitial that persists (GFS_control_type). That's where I put all of the module variables that were causing problems with multiples instances.

Ok, that makes sense. Different terminology ;-)

@dustinswales
Copy link
Collaborator

@climbfuji Sorry, I have this (bad) habit of calling everything that comes into the physics an Interstitial, even though Interstitials are a diverse group.

@climbfuji
Copy link
Collaborator

Can we push this across the finish line, please?

@areinecke
Copy link

Can we get a status update on this?

@climbfuji
Copy link
Collaborator

Can we get a status update on this?

Based on the conversation that followed #1000 (comment), only one additional change is needed for GFS_phys_time_vary, following what @dustinswales did for mp_thompson. I can do this today with a PR that can be tested in the UFS (because the code needs to go into NCAR main first before we pull it down). Then this PR can be closed as "merged similar functionality in a number of separate PRs".

@climbfuji
Copy link
Collaborator

Can we get a status update on this?

Based on the conversation that followed #1000 (comment), only one additional change is needed for GFS_phys_time_vary, following what @dustinswales did for mp_thompson. I can do this today with a PR that can be tested in the UFS (because the code needs to go into NCAR main first before we pull it down). Then this PR can be closed as "merged similar functionality in a number of separate PRs".

I'll have this fixed shortly. While looking at it, I also found a bug in the mp_thompson implementation that I am going fix in the PR.

This was referenced Dec 19, 2024
Open
Open
Open
@climbfuji
Copy link
Collaborator

Can we get a status update on this?

Based on the conversation that followed #1000 (comment), only one additional change is needed for GFS_phys_time_vary, following what @dustinswales did for mp_thompson. I can do this today with a PR that can be tested in the UFS (because the code needs to go into NCAR main first before we pull it down). Then this PR can be closed as "merged similar functionality in a number of separate PRs".

I'll have this fixed shortly. While looking at it, I also found a bug in the mp_thompson implementation that I am going fix in the PR.

All done: ufs-community#242

The associated ufs-weather-model PR will close this PR as completed, even though technically the ccpp-physics changes still need to transpire from the ufs-community/ufs-dev branch to NCAR/main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dustinswales dustinswales dustinswales left review comments

@climbfuji climbfuji climbfuji left review comments

@grantfirl grantfirl Awaiting requested review from grantfirl grantfirl is a code owner

@ChunxiZhang-NOAA ChunxiZhang-NOAA Awaiting requested review from ChunxiZhang-NOAA

@gthompsnWRF gthompsnWRF Awaiting requested review from gthompsnWRF

@RuiyuSun RuiyuSun Awaiting requested review from RuiyuSun

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michalakes @climbfuji @grantfirl @dustinswales @areinecke