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.

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

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

Merged
merged 36 commits into from
Oct 7, 2022

Conversation

grantfirl
Copy link
Collaborator

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

AnningCheng-NOAA and others added 30 commits August 23, 2021 13:34
Update to mraerosol branch from Dom
…11018

Anning mraerosol updates dom 20211018
@grantfirl
Copy link
Collaborator Author

This PR brings the changes from ufs-community#2 into the NCAR/main branch. See ufs-community/ufs-weather-model#1438 for UFS regression testing.

@grantfirl
Copy link
Collaborator Author

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

@grantfirl
Copy link
Collaborator Author

grantfirl commented Oct 4, 2022

@gthompsnWRF
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

@gthompsnWRF Given that ufs-community#1 has yet to be finally tested and merged, I will make the documentation changes to ufs-community#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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

grantfirl commented Oct 6, 2022

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

@gthompsnWRF
Copy link
Collaborator

@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 (ufs-community#2 (comment)) and it refers to the comment just above by Anning that was not incorporated into the PR before merging.

@dustinswales
Copy link
Collaborator

@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
Copy link
Collaborator Author

@gthompsnWRF Thanks for the clarification. I have added Anning's additional documentation to ufs-community@9009159.

@dustinswales dustinswales merged commit 2550352 into NCAR:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants