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

Smoke/Dust updates for RRFS code freeze #135

Merged
merged 6 commits into from
Dec 27, 2023

Conversation

haiqinli
Copy link
Collaborator

@haiqinli haiqinli commented Dec 3, 2023

  1. Include convective transport and wet removal of smoke/dust in the GF convection.
  2. A new dry deposition option for smoke/dust.
  3. Update the diurnal cycle of biomass burning emissions.
  4. Clean the smoke code.

@lisa-bengtsson
Copy link
Collaborator

Hi Haiqin, should these updates be in C3 as well?

@haiqinli
Copy link
Collaborator Author

haiqinli commented Dec 4, 2023

Hi Haiqin, should these updates be in C3 as well?

Hi Lisa, yes, we will include these updates in C3 when we have more time to test it.

Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes are needed wrt the precision, but otherwise looks fine to me.

physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_deep.F90 Outdated Show resolved Hide resolved
physics/cu_gf_deep.F90 Outdated Show resolved Hide resolved
physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/smoke_dust/module_add_emiss_burn.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/module_add_emiss_burn.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/module_add_emiss_burn.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/module_add_emiss_burn.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/plume_data_mod.F90 Outdated Show resolved Hide resolved
@dustinswales
Copy link
Collaborator

Since this touches GPU functionality in the GF scheme, I think it's a good idea to have one of the GPU developers review the proposed changes (@timsliwinski-noaa).

Copy link

@danielabdi-noaa danielabdi-noaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dustinswales I have made a few obvious suggestions to make it GPU complaint, but this is an extensive set of changes that really need to be tested to figure out all the GPU related issues. New smoke related modules may need to be ported to the GPU as well. Adding OpenACC pragmas, even incorrect ones, will not break the CPU code so it is safe in that regard.

physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_driver.F90 Show resolved Hide resolved
physics/cu_gf_driver_post.F90 Show resolved Hide resolved
physics/cu_gf_driver_post.F90 Outdated Show resolved Hide resolved
physics/cu_gf_driver_pre.F90 Show resolved Hide resolved
physics/cu_gf_driver_pre.F90 Outdated Show resolved Hide resolved
@grantfirl
Copy link
Collaborator

@haiqinli When is the RRFS code freeze and when does this need to be merged? I'm concerned that there are a lot of requested changes and testing the GPU fixes isn't even accomplished through the normal RT process.

@dustinswales
Copy link
Collaborator

@danielabdi-noaa Thanks for your review.
@grantfirl As you mentioned, the GPU concerns are a whole other can of worms, which we are still figuring out how to test.
From my understanding, the GF scheme's GPU functionality is currently not working and Daniel is working on that.
Should we accept these recommendation from Daniel in this PR? Or include them in a follow up PR with other tested (in the SCM) changes? (And maybe(?) we can harden up the scripts to easily run the SCM GPU tests on Hera?)

Copy link
Collaborator

@mkavulich mkavulich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggested changes, mostly minor

physics/cu_gf_deep.F90 Outdated Show resolved Hide resolved
physics/cu_gf_deep.F90 Outdated Show resolved Hide resolved
physics/cu_gf_deep.F90 Show resolved Hide resolved
physics/cu_gf_driver.meta Outdated Show resolved Hide resolved
physics/cu_gf_driver_post.meta Outdated Show resolved Hide resolved
physics/smoke_dust/dep_dry_simple_mod.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/rrfs_smoke_wrapper.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/rrfs_smoke_wrapper.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/rrfs_smoke_wrapper.F90 Outdated Show resolved Hide resolved
physics/smoke_dust/rrfs_smoke_wrapper.F90 Outdated Show resolved Hide resolved
@haiqinli
Copy link
Collaborator Author

@haiqinli When is the RRFS code freeze and when does this need to be merged? I'm concerned that there are a lot of requested changes and testing the GPU fixes isn't even accomplished through the normal RT process.

@grantfirl In the RRFS-SD PR, which uses the vegfrac from RUC LSM. However, the conus13km_debug_intel case crashed when using vegfrac from RUC LSM. We just figured it out that the vegfrac is not available in the regression test run case of conus13km_debug_intel/INPUT/oro_data.nc. I will redo the regression test and mark the reason of crashed cases like conus13km_debug_intel in the PR. It is expected to be ready tomorrow morning when the regression test is done. Thanks.

physics/cu_gf_driver.F90 Outdated Show resolved Hide resolved
@jkbk2004
Copy link

@haiqinli can you resolve the suggestions to get final approvals on this pr?

@haiqinli
Copy link
Collaborator Author

@haiqinli can you resolve the suggestions to get final approvals on this pr?

Sure, I am working on it, and should be done soon. Thanks.

@grantfirl
Copy link
Collaborator

@haiqinli As long as you push the change related to the intent of nchem, that is good enough. I'm sorry to review so late. I'll approve and resolve the comments so that we can move this along for your deadline. Thanks for getting the GPU changes in, although we (CCPP code managers) have no way to test this right now.

@grantfirl grantfirl dismissed stale reviews from mkavulich and dustinswales December 20, 2023 17:13

Comments have been addressed.

@haiqinli
Copy link
Collaborator Author

@grantfirl Thank you very much for your help. The PR has been updated to include nchem intent declaration.

@jkbk2004
Copy link

All tests are done and confirmed at ufs-community/ufs-weather-model#2024. @grantfirl @Qingfu-Liu @dustinswales Can you merge in this pr?

@dustinswales dustinswales merged commit df9e1ad into ufs-community:ufs/dev Dec 27, 2023
2 of 3 checks passed
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.

8 participants