search

NCAR / ccpp-physics

GFS physics for CCPP
Other
56 stars 145 forks source link

Bug fix for rain evaporation; there was a units error due to air density #1014

Closed gthompsnWRF closed 1 year ago

gthompsnWRF commented 1 year ago

Calculations of rain evaporation rate (variable called prv_rev) had some mismatching of units due to air density inconsistencies. The set of 3 lines will fix it causing prv_rev to be kg kg-1 s-1 as it should be ahead of adding it to the principle tendency terms.

This was discovered with the help of Ruiyu Sun and Eric Aligo (NCEP-EMC) due to discovering a few grid points with larger than 100% RH (with respect to water) due to the improper rate limiter.

grantfirl commented 1 year ago

@gthompsnWRF This fix is targeted for UFS applications, correct? If so, it's probably best to target the https://github.com/ufs-community/ccpp-physics/tree/ufs/dev branch for a pull request. It will find its way back to the NCAR:main branch after going through the UFS merge queue, which is pretty short right now.

gthompsnWRF commented 1 year ago

@gthompsnWRF This fix is targeted for UFS applications, correct? If so, it's probably best to target the https://github.com/ufs-community/ccpp-physics/tree/ufs/dev branch for a pull request. It will find its way back to the NCAR:main branch after going through the UFS merge queue, which is pretty short right now.

Thanks Grant. Should I close this one and just do a quick repeat in the other?

grantfirl commented 1 year ago

@gthompsnWRF This fix is targeted for UFS applications, correct? If so, it's probably best to target the https://github.com/ufs-community/ccpp-physics/tree/ufs/dev branch for a pull request. It will find its way back to the NCAR:main branch after going through the UFS merge queue, which is pretty short right now.

Thanks Grant. Should I close this one and just do a quick repeat in the other?

Ideally, yes. @ericaligo-NOAA and @RuiyuSun , please review/approve over there when it is recreated.

gthompsnWRF commented 1 year ago

Closing this PR per the conversation above.

gthompsnWRF commented 1 year ago

@grantfirl Apparently, I don't know how to aim my PR into ufs-community/ccpp instead of the NCAR one; because when I attempt to change the target repo, I get what is shown in this screenshot. I forked my repo from the NCAR one, not the ufs one and I cannot make another fork. So I'm lost what to do.

Screen Shot 2023-04-26 at 1 09 56 PM
grantfirl commented 1 year ago

I think you're doing it correctly, but choose the ufs/dev branch in the ufs-community fork instead of main. Since the ufs/dev branch is a couple of PRs ahead and if you started from NCAR:main, you may need to pull down the latest ufs/dev branch and merge it into [bug_fix_prv_rev](https://github.com/gthompsnWRF/ccpp-physics/tree/bug_fix_prv_rev) before/after you create the new PR.

grantfirl commented 1 year ago

You should be able to initiate PRs from your fork, even if it was forked from NCAR. It's the same situation I'm in actually, and I have no trouble submitting PRs.

gthompsnWRF commented 1 year ago

@grantfirl I clearly am still a unsophisticated Git user. I don't know how to update my branch (within my fork) to merge in the ufs/dev branch. Like a fool, I just tried git checkout ufs/dev when in my directory from where I cloned my fork. So I need more hand-holding please to align things properly. But I did set the base branch so I created the PR. Just got to merge in all the other stuff.

grantfirl commented 1 year ago

@gthompsnWRF OK, not a problem. Let's try this:

  1. If you're able to checkout ufs/dev, I'm assuming that if you do a git remote -v, you will see https://github.com/ufs-community/ccpp-physics listed as one of your remotes. If not, do git remote add ufs-fork https://github.com/ufs-community/ccpp-physics. Once it is listed as a remote, go ahead and do a git fetch whatever_your_fork_of_ufs-community/ccpp-physics_is_named and git pull whatever_your_fork_of_ufs-community/ccpp-physics_is_named ufs/dev. This should pull down the latest ufs/dev branch. If you do a git log, you should see https://github.com/ufs-community/ccpp-physics/commit/eda81a58a1e3dd87c945f0d9cc43d53a1c858d65 at the top.
  2. Now that your ufs/dev branch is up-to-date, git checkout bug_fix_prv_rev and merge in the latest ufs/dev: git merge ufs/dev. There should very likely not be any conflicts and it should succeed.
  3. Once ufs/dev has been merged in, git push should update your branch on your GitHub fork and the PR will automatically update.

There will be a bunch of commits associated with your PR since you started from NCAR:main, which has a slightly different history (due to most code going into ufs/dev first, then NCAR:main), but it's fine. It's possible that there will still be additional files that will have changed in NCAR:main in addition to your scheme file. We can choose to revert those if we want.

gthompsnWRF commented 1 year ago

As usual, still very lost. The add remote worked such that when I use git remote -v I am now seeing both my fork and the ufs one:

origin  https://github.com/gthompsnWRF/ccpp-physics.git (fetch)
origin  https://github.com/gthompsnWRF/ccpp-physics.git (push)
ufs-fork    https://github.com/ufs-community/ccpp-physics (fetch)
ufs-fork    https://github.com/ufs-community/ccpp-physics (push)

But, following what I believe is your instruction, when I fetch using: gthompsnWRF/ccpp-physics, I get a failure:

fatal: Could not read from remote repository

And remember, my fork was coming from NCAR not ufs-community, so I don't have what you said: "git fetch whatever_your_fork_of_ufs-community/ccpp-physics_is_named" because it's not a fork of ufs-community. Then I tried: git pull ufs-fork ufs/dev and it certainly updated a lot of files.

But, then I git checkout _my_branch_ and try a git merge ufs/dev but it fails with: merge: ufs/dev - not something we can merge

So it seems things are still not right. :(

grantfirl commented 1 year ago

Hmm. Do you know which branch you had checked out with you did the git pull ufs-fork ufs/dev? I think that was the missing piece. I forgot to include making sure that you're in the ufs/dev branch before the git pull in the instructions. If you git checkout ufs/dev, does it work? If so, does it have https://github.com/ufs-community/ccpp-physics/commit/eda81a58a1e3dd87c945f0d9cc43d53a1c858d65 as the latest commit? If not, do git fetch ufs-fork and then try to git checkout ufs/dev again and check for the latest commit. Once you're able to check out ufs/dev, I would think that you'd be able to merge it following 2 and 3 from the previous comment.

grantfirl commented 1 year ago

If this doesn't work, you can always start over by git clone https://github.com/ufs-community/ccpp-physics, cd into ccpp-physics, git checkout ufs/dev, git checkout -b _my_new_branch_name_, edit or copy in the edited file, git add _your_file_, git commit, git remote add greg-fork https://github.com/gthompsnWRF/ccpp-physics, git push -u _my_new_branch_name_, and redo the PR again.