search

ESCOMP / PUMAS

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

Potential concern about the newly added "implicit_fall" subroutine #30

Closed sjsprecious closed 2 years ago

sjsprecious commented 2 years ago

In the latest v1.18 version of PUMAS code (https://github.com/ESCOMP/PUMAS/tree/pumas_cam-release_v1.18), there is a mismatch of dimension for a subroutine argument:

Therefore, due to the mismatch of dimension, the value of ze(nlev+1) may not be set correctly when the implicit_fall subroutine is called. I am not sure whether this could lead to unexpected behaviour for certain compilers/machines. However, I do find that changing the 1-D call of implicit_fall subroutine to a 2-D call is an answer-changing revision, which is unexpected and I suspect that it is related to this mismatch.

andrewgettelman commented 2 years ago

Thanks Jian,

It looks like this is probably a mistake, and we can possibly eliminate ze and calculate dz directly (from pdel) for each level and use that. I will have to test it. This will take a few days (or maybe into next week). More soon.

sjsprecious commented 2 years ago

Thanks Jian,

It looks like this is probably a mistake, and we can possibly eliminate ze and calculate dz directly (from pdel) for each level and use that. I will have to test it. This will take a few days (or maybe into next week). More soon.

Thanks Andrew. Your solution sounds great and I could wait for your revision before I issue my PR. The OpenACC work is already done except for the implicit_fall subroutine.

andrewgettelman commented 2 years ago

I have a solution, and am testing it now. Single column and global. I'll put it in the code once I verify it is tested. One minor issue I came up with that I might need to ask about is CAM pint does not seem to be valid at the surface (so I need to use pseudo surface pressure). Not elegant, but it works. More soon.

andrewgettelman commented 2 years ago

Okay, I have tested and sorted out the update with help from Steve. @sjsprecious, do you want to take a look at the latest code. I think I was able to push it back, but since it is both CAM and PUMAS code, I am not sure. Maybe @Katetc can verify I did things right. I modified pumas/micro_mg3_0.F90 and cam/micro_mg_cam.F90, put in a commit from outside of the pumas directory and inside the pumas directory. I then did a git push from outside of the pumas directory.

A push attempt from inside the pumas directory gives me:

git push fatal: You are not currently on a branch. To push the history leading to the current (detached HEAD) state now, use

git push origin HEAD:<name-of-remote-branch>

And git status gives me: $ git status On branch gettelman_pumas_update0821 Your branch is up to date with 'origin/gettelman_pumas_update0821'.

nothing to commit, working tree clean (base) -bash-4.2$ cd pumas/ (base) -bash-4.2$ git status HEAD detached from pumas_cam-release_v1.18 nothing to commit, working tree clean

Do I still need to do something to push changes back to PUMAS?

Thanks for clarifying, sorry for the stupid questions.

andrewgettelman commented 2 years ago

Once this is sorted out, we can probably close this issue.

sjsprecious commented 2 years ago

Okay, I have tested and sorted out the update with help from Steve. @sjsprecious, do you want to take a look at the latest code. I think I was able to push it back, but since it is both CAM and PUMAS code, I am not sure. Maybe @Katetc can verify I did things right. I modified pumas/micro_mg3_0.F90 and cam/micro_mg_cam.F90, put in a commit from outside of the pumas directory and inside the pumas directory. I then did a git push from outside of the pumas directory.

A push attempt from inside the pumas directory gives me:

git push fatal: You are not currently on a branch. To push the history leading to the current (detached HEAD) state now, use

git push origin HEAD:<name-of-remote-branch>

And git status gives me: $ git status On branch gettelman_pumas_update0821 Your branch is up to date with 'origin/gettelman_pumas_update0821'.

nothing to commit, working tree clean (base) -bash-4.2$ cd pumas/ (base) -bash-4.2$ git status HEAD detached from pumas_cam-release_v1.18 nothing to commit, working tree clean

Do I still need to do something to push changes back to PUMAS?

Thanks for clarifying, sorry for the stupid questions.

Thanks @andrewgettelman for your update. It is great that there is already a solution tested for this issue!

Are you pushing your CAM change to your gettelman_pumas_update0821 branch in https://github.com/PUMASDevelopment/CAM and your PUMAS change to the release/cam branch in https://github.com/ESCOMP/PUMAS? I think you probably need to do something to push changes back to PUMAS since you are working on two different repos but @Katetc may clarify that better.

Once the GitHub issue is sorted out, I would merge your changes into my work branch and continue my GPU porting later. Thanks again for your time and help!

andrewgettelman commented 2 years ago

Yes, trying to sort that out. More soon....

andrewgettelman commented 2 years ago

Okay, this is now updated on the gettelman_pumas_mods0821 branch, and that could now has all the latest changes for the implicit fall speed update.