search

NCAR / ccpp-physics

GFS physics for CCPP
Other
56 stars 145 forks source link

UFS/dev PR#2 (Coupling Merra2 aerosol climatology and GOCART forecasted aerosol with Thompson microphysics) #971

Closed grantfirl closed 1 year ago

grantfirl commented 1 year ago

All credit for code changes and description to Anning Cheng.

MERRA2 climatological aerosols are used to calculate number concentrations of ice friendly and liquid friendly aerosols. IN and CCN can be activated by using aerosols. The indirect interactions of aerosols and clouds are expected to happen. This PR is created to replace the NCAR/ccpp-physics PR#720, which was created by Anning Cheng.

The MERRA2 coupled Thompson (EXP MRAERO) shows improvement from the control run and the forecasted NIFA and NWFA approach (LTAERO), specially decreased cloud water bias as shown in the attached poster. More comprehensive comparison located at https://www.emc.ncep.noaa.gov/gmb/acheng/mraero/.

Because the GOCART coupled Thompson scheme shares the same IN/CCN activation module as the MERRA2 coupled Thompson scheme, we expect more improvements. Some results can be found here: gass_3_v1.pdf

grantfirl commented 1 year ago

This PR brings the changes from https://github.com/ufs-community/ccpp-physics/pull/2 into the NCAR/main branch. See https://github.com/ufs-community/ufs-weather-model/pull/1438 for UFS regression testing.

grantfirl commented 1 year ago

Note to all potential reviewers. If you already reviewed https://github.com/ufs-community/ccpp-physics/pull/2, then there is no reason to re-review this. It is the exact same PR, just now going to NCAR/main.

grantfirl commented 1 year ago

Associated PRs: https://github.com/NCAR/ccpp-physics/pull/971 https://github.com/NCAR/fv3atm/pull/68 https://github.com/NCAR/ufs-weather-model/pull/69 https://github.com/NCAR/ccpp-scm/pull/346

gthompsnWRF commented 1 year ago

@grantfirl How come we cannot just undo the commit made by Chunxi and fix the only remaining comment I keep making about the full documentation of how MERRA aerosols are input into the NWFA/NIFA variables? This is trivial and yet it will help myself and others to understand what Anning has done. I'm referring to line#881 of mp_thompson.F90 where I nearly had to beg to get the subroutine more fully documented.

dustinswales commented 1 year ago

@gthompsnWRF This PR is just to keep the NCAR/ccpp-physics:main and ufs-community/ccpp-physics:ufs/dev branches up-to-date. The commit made by @ChunxiZhang-NOAA has already been merged in ufs-community/ccpp-physics:ufs/dev.

This is a new step in the code management of the two forks, where the code managers open up PRs between the forks to keep them "synced". This happens ~ every two weeks, and AFTER code changes have gone through the formal review process. So this is not the time to add in new changes.

We are working on a way to not alert the CODEOWNERS for this second PR. Sorry for the false github alarm.

gthompsnWRF commented 1 year ago

@gthompsnWRF This PR is just to keep the NCAR/ccpp-physics:main and ufs-community/ccpp-physics:ufs/dev branches up-to-date. The commit made by @ChunxiZhang-NOAA has already been merged in ufs-community/ccpp-physics:ufs/dev.

This is a new step in the code management of the two forks, where the code managers open up PRs between the forks to keep them "synced". This happens ~ every two weeks, and AFTER code changes have gone through the formal review process. So this is not the time to add in new changes.

We are working on a way to not alert the CODEOWNERS for this second PR. Sorry for the false github alarm.

I understand how this works, but there was a seemingly simple solution to use GitHub revert on a merge and then make a simple single file comment section change, then re-merge again. I guess the point here is paying closer attention to review requests for changes and meeting the requests before a merge happens. To me, this happened prematurely and I feel as though I was "begging" for more info before a merge. Then the merge happened anyway. Was I really asking too much to be documented?? I think not given its ambiguities.

grantfirl commented 1 year ago

@gthompsnWRF Given that https://github.com/ufs-community/ccpp-physics/pull/1 has yet to be finally tested and merged, I will make the documentation changes to https://github.com/ufs-community/ccpp-physics/pull/1. Regarding the removal of the lines 2208 and 3 similar lines, this will be diverging from the latest WRF code. I'll put this change in as its own commit so that it can be easily reverted if necessary. This may be a little messy code management-wise, since we'll be adding two unrelated changes in that PR that both change the answer (the new ice aloft tuning and the removal of those lines). @ChunxiZhang-NOAA requested adding an issue in the ufs-community physics to track this too, which I will, so that others can comment since I don't think that everyone is convinced that the lines should be removed.

grantfirl commented 1 year ago

@gthompsnWRF Regarding why not make the change here, we're trying to keep PRs into ufs-community fork; ufs/dev branch and NCAR fork/ main branch identical for code management purposes.

grantfirl commented 1 year ago

@gthompsnWRF This PR is just to keep the NCAR/ccpp-physics:main and ufs-community/ccpp-physics:ufs/dev branches up-to-date. The commit made by @ChunxiZhang-NOAA has already been merged in ufs-community/ccpp-physics:ufs/dev. This is a new step in the code management of the two forks, where the code managers open up PRs between the forks to keep them "synced". This happens ~ every two weeks, and AFTER code changes have gone through the formal review process. So this is not the time to add in new changes. We are working on a way to not alert the CODEOWNERS for this second PR. Sorry for the false github alarm.

I understand how this works, but there was a seemingly simple solution to use GitHub revert on a merge and then make a simple single file comment section change, then re-merge again. I guess the point here is paying closer attention to review requests for changes and meeting the requests before a merge happens. To me, this happened prematurely and I feel as though I was "begging" for more info before a merge. Then the merge happened anyway. Was I really asking too much to be documented?? I think not given its ambiguities.

@gthompsnWRF To be clear, you're only referring to the documentation change in https://github.com/NCAR/ccpp-physics/commit/d49e0d644f2da5c04852d81460322219dcf2149c? Do you still want the resetting of nc lines removed (e.g. 2208 in module_mp_thompson)?

gthompsnWRF commented 1 year ago

@gthompsnWRF This PR is just to keep the NCAR/ccpp-physics:main and ufs-community/ccpp-physics:ufs/dev branches up-to-date. The commit made by @ChunxiZhang-NOAA has already been merged in ufs-community/ccpp-physics:ufs/dev. This is a new step in the code management of the two forks, where the code managers open up PRs between the forks to keep them "synced". This happens ~ every two weeks, and AFTER code changes have gone through the formal review process. So this is not the time to add in new changes. We are working on a way to not alert the CODEOWNERS for this second PR. Sorry for the false github alarm.

I understand how this works, but there was a seemingly simple solution to use GitHub revert on a merge and then make a simple single file comment section change, then re-merge again. I guess the point here is paying closer attention to review requests for changes and meeting the requests before a merge happens. To me, this happened prematurely and I feel as though I was "begging" for more info before a merge. Then the merge happened anyway. Was I really asking too much to be documented?? I think not given its ambiguities.

@gthompsnWRF To be clear, you're only referring to the documentation change in d49e0d6? Do you still want the resetting of nc lines removed (e.g. 2208 in module_mp_thompson)?

@grantfirl To be extremely concise, my comment is seen in this specific comment link (https://github.com/ufs-community/ccpp-physics/pull/2#issuecomment-1263889216) and it refers to the comment just above by Anning that was not incorporated into the PR before merging.

dustinswales commented 1 year ago

@gthompsnWRF There was a consensus to go ahead and merge PR#2 and address your concerns afterwards.

@grantfirl @ChunxiZhang-NOAA or myself will work with you to ensure this gets added imminently, but not in this PR.

grantfirl commented 1 year ago

@gthompsnWRF Thanks for the clarification. I have added Anning's additional documentation to https://github.com/ufs-community/ccpp-physics/pull/1/commits/9009159abc259bce38c59b1dbc51f8cd54f6544b.