search

ESCOMP / PUMAS

Parameterization for Unified Microphysics Across Scales
9 stars 12 forks source link

Port updated PUMAS code to GPU and fix a typo in the code #34

Closed sjsprecious closed 2 years ago

sjsprecious commented 2 years ago

In this PR, we add the OpenACC directives to the recently revised PUMAS code (see details at https://github.com/ESCOMP/PUMAS/tree/gettelman_pumas_mods0821) so that the PUMAS parameterization could remain GPU-compatible. We also refactor the code to make the interface of implicit sedimentation easier for further porting and implement a 2-D version for better performance. In addition, we fix a typo found in this issue: https://github.com/ESCOMP/PUMAS/issues/32.

The detailed changes include:

Note that pushing the column loop into the level loop the change in the implicit_fall subroutine will fail both the CAM standard test suite (CPU run on Cheyenne) and ECT (GPU run on Casper). The detailed issue comes from the level-dependent calculation of qm and m1 variables, which is column-independent. I do not know the exact underlying cause and thus highlight the current implementation (which will pass both CAM test suite and ECT) to be revisited in the future.

Test suite: [aux_cam] Test baseline: generated with CAM codes from https://github.com/PUMASDevelopment/CAM/tree/gettelman_pumas_update0821 and PUMAS codes from https://github.com/ESCOMP/PUMAS/tree/gettelman_pumas_mods0821, cime tag 5.8.44. Test machine: [Cheyenne] Test result: [BFB]

andrewgettelman commented 2 years ago

@sjsprecious , thanks very much for these changes. Are the current changes BFB in the CPU code? It's fine if not, I just want to make sure to test the CPU code quickly to verify climate if they are not BFB. Thanks!

sjsprecious commented 2 years ago

@sjsprecious , thanks very much for these changes. Are the current changes BFB in the CPU code? It's fine if not, I just want to make sure to test the CPU code quickly to verify climate if they are not BFB. Thanks!

Hi @andrewgettelman , thanks for your comment. Yes, all the changes are BFB against your latest PUMAS revision (https://github.com/ESCOMP/PUMAS/tree/gettelman_pumas_mods0821) on Cheyenne, according to the CAM test suite.

andrewgettelman commented 2 years ago

Hi @sjsprecious. I looked in detail at the code, and talked with Hugh Morrison about it. We like the new sedimentation interfaces. However, for modularity purposes, it would be best if these interfaces separated mass and number and are called twice for each class instead of once.

There are two reasons for this. (A) When we have fixed number, we do not need to sediment number, (B) as we evolve the scheme we want to be able to more flexibly modify the 'moments' (mass and number), for some implementations, we may have a 3rd moment ('skewness' or 'ice shape') to sediment. Thus we may want to have 1, 2 or even 3 calls to the sedimentation for each species.

This would apply to both the existing and implicit sedimentation.

It does not look like this would be too hard to modify, to just shrink the interfaces and call them twice.

Thanks for the help.

Regards,

Andrew

sjsprecious commented 2 years ago

Hi @sjsprecious. I looked in detail at the code, and talked with Hugh Morrison about it. We like the new sedimentation interfaces. However, for modularity purposes, it would be best if these interfaces separated mass and number and are called twice for each class instead of once.

There are two reasons for this. (A) When we have fixed number, we do not need to sediment number, (B) as we evolve the scheme we want to be able to more flexibly modify the 'moments' (mass and number), for some implementations, we may have a 3rd moment ('skewness' or 'ice shape') to sediment. Thus we may want to have 1, 2 or even 3 calls to the sedimentation for each species.

This would apply to both the existing and implicit sedimentation.

It does not look like this would be too hard to modify, to just shrink the interfaces and call them twice.

Thanks for the help.

Regards,

Andrew

Hi @andrewgettelman , thanks for your comment and detailed explanation. I will revise the existing and implicit sedimentation subroutines to separate the number and mass sediment.

Once the revision is done and passes the BFB test, I will ask you all for review again.

andrewgettelman commented 2 years ago

Thanks @sjsprecious. Sorry for the additional work, but I think this will be pretty quick to do. Would be good to do for both the sedimentation and the implicit sedimentation.

Thanks again for all your help!

sjsprecious commented 2 years ago

Hi @sjsprecious . Thanks very much for doing this. I think it's a nice balance between simplicity and modularity of the code.

One question: why do the interfaces need to know whether it is a qx or nx tendency? Does it matter, or could the code be simplified a bit? I recognize the sedimentation tendency (sedtend) does not exist for number, but why do you have to pass qxtend or nxtend. Seems like they are treated equivalently. Or maybe I missed something.

Regards,

Andrew

Hi @andrewgettelman , thanks for your comment and you do not miss anything here. Originally I kept qx and nx as they were calculated together in the same subroutine. But since number and mass sediments are called separately now and the tendency form does look equivalently, I think you are right that the code could be further simplified by introducing an xxtend for both qxtend and nxtend.

If you think it sounds good, I would just need to run another BFB test and push the changes later. Thanks for catching it!

andrewgettelman commented 2 years ago

Thanks @sjsprecious. I think that sounds good and would make the routine more generic. That would be great.

sjsprecious commented 2 years ago

Thanks @sjsprecious. I think that sounds good and would make the routine more generic. That would be great.

Thanks @andrewgettelman . I just pushed the change of xxtend. It passed the BFB test for CPU run on Cheyenne and ECT for GPU run on Casper.

Katetc commented 2 years ago

Hi guys, I did a review this morning and this looks good to me. I see that Jian has based this branch off Andrew's branch with the implicit fall correction, which is great. Now we could either merge Andrew's branch and then this one, or just merge this branch and delete Andrew's. I like the second option because it's fewer steps. Andrew, are you comfortable with bringing in your changes now? If so, I'll start testing and do the merge when they are done.

andrewgettelman commented 2 years ago

Yes, fine to bring in my changes now and start testing. Thanks @Katetc

sjsprecious commented 2 years ago

Thanks @andrewgettelman and @Katetc for your time and help.

Once this PR is merged and tagged, I and @johnmauff would like to start to work on the GPU regression test for PUMAS code. This may need to merge Andrew's CAM change (https://github.com/PUMASDevelopment/CAM/tree/gettelman_pumas_update0821) into the cam_development branch and make a new CAM tag first. Do you have an idea about when this could be done?

Katetc commented 2 years ago

I added a Tag_Notes document that we can update as we develop any dependancies or issues. Currently, I just made a note about the fact that this tag requires Andrew's branch in PUMASdevelopment which is not on the cam_development trunk yet.

For that change to be merged, we are waiting on Adam Herrington's changes to the tphysbc ordering, and we will bring in all of these new PUMAS changes as a secondary microphysics in a physics package to be developed towards CAM7. So it will probably still be a while.