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

cam6_4_019: New CLUBB External to fix GPU problem #1086

Merged
merged 28 commits into from
Aug 12, 2024

Conversation

Katetc
Copy link
Collaborator

@Katetc Katetc commented Jul 9, 2024

Small answer changes, but no new CLUBB science. Diagnostics can be found here:
https://webext.cgd.ucar.edu/F2000climo/newCLUBBtesting/larson_tag_20240605.katemerge.062724-0201.F2000dev.f09_f09_mg17_1_2_vs_larson_tag_control.cam6_3_162.062724-1359.F2000dev.f09_f09_mg17_1_2/website/index.html

Ran 6 year ne30 BLT1850 simulations to look at differences in applicable and longer runs. Had some trouble with diagnostics but the results can be found here:
https://webext.cgd.ucar.edu/FHIST/clubb_tests/larson_tag_20240605.katemerge.071924-1633.FLTHIST.ne30pg3_g17_1979_1984_vs_larson_tag_control.cam6_4_007.071924-1639.FLTHIST.ne30pg3_g17_1979_1984/website/

Differences seem to be minimal.

Fixes #1036
closes #1048

src/physics/cam/clubb_intr.F90 Outdated Show resolved Hide resolved
src/physics/cam/clubb_intr.F90 Outdated Show resolved Hide resolved
src/physics/cam/subcol_SILHS.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@sjsprecious sjsprecious left a comment

Choose a reason for hiding this comment

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

The current CLUBB GPU tag runs successfully on Derecho's GPU but fails the ensemble consistency test. The UWM group is debugging the GPU code now but we (CISL group) are fine with moving on with this PR and including the new CLUBB GPU tag in the CAM regression test.

@adamrher
Copy link

@Katetc I'm under the impression that these new clubb externals remove the ghost point from the thermodynamic 'zt' grid. But I'm not seeing a reduction in the size of the zt arrays in clubb_intr in this PR. What am I missing?

.gitmodules Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

The ccs_config tag needs to be updated to ccs_config_cesm1.0.0 so that the CLUBB GPU code can be compiled correctly on the GPU.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current head of cam_development points to ccs_config_cesm1.0.0 so when this branch is merged up, the external will be correctly versioned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Kate. That is great!

@@ -1810,18 +1850,25 @@ subroutine clubb_ini_cam(pbuf2d)
call addfld ( 'edmf_qtflx' , (/ 'ilev' /), 'A', 'W/m2' , 'qt flux (EDMF)' )
end if

#ifndef SILHS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't this be wrapped in a if ( trim(subcol_scheme) == 'SILHS' ) then? The only reason to put code like this inside a #ifndef directive is to hide it completely from the compiler. I believe that hm_metadata and the two fields always are declared.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like it should be fine. I'm updating to just a straight "if" here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably use /= instead of .ne. to adhere to the CAM coding standards (it is a should and not a must)

src/physics/cam/clubb_intr.F90 Outdated Show resolved Hide resolved
@cacraigucar
Copy link
Collaborator

@adamrher and @bstephens82 - @Katetc requested reviews from both of you. Please either remove yourself as a reviewer or do a review when you can.

Copy link

@adamrher adamrher left a comment

Choose a reason for hiding this comment

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

I'm approving this PR, but would like to understand why all the arrays on the thermodynamic grid in clubb_intr.F90 are still retaining their ghost points. It was my understanding that this PR is removing the ghost point? Is the plan to just do that in the clubb externals, not the CAM interface?

For example, rho_zt is still of size dimension(state%ncol,pverp+1-top_lev) in clubb_intr.F90; it has the ghost point.

@vlarson
Copy link

vlarson commented Jul 26, 2024

I'm approving this PR, but would like to understand why all the arrays on the thermodynamic grid in clubb_intr.F90 are still retaining their ghost points. It was my understanding that this PR is removing the ghost point? Is the plan to just do that in the clubb externals, not the CAM interface?

For example, rho_zt is still of size dimension(state%ncol,pverp+1-top_lev) in clubb_intr.F90; it has the ghost point.

This code merely changes some lower BCs to one-sided derivatives within CLUBB core (and fixes some minor bugs involving full-level variables being placed on the wrong (half-level) grid levels). This PR doesn't change the clubb_intr, but UWM hopes to do that at some point.

@vlarson
Copy link

vlarson commented Jul 26, 2024

@adamrher, do you think that the changes in the solution look "large", or do they look like what you'd expect from truncation error (numerics)?

@adamrher
Copy link

Thanks for getting back to me on the ghost point. All is good then.

@adamrher, do you think that the changes in the solution look "large", or do they look like what you'd expect from truncation error (numerics)?

The global mean turbulent fluxes and radiative fluxes look ~identical, so that suggests round-off level changes. There's some differences in the lat-lon map plots but those are what you would expect from comparing to two, 2 year long runs.

Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

One minor tweak requested, but I am approving

@@ -1810,18 +1850,25 @@ subroutine clubb_ini_cam(pbuf2d)
call addfld ( 'edmf_qtflx' , (/ 'ilev' /), 'A', 'W/m2' , 'qt flux (EDMF)' )
end if

#ifndef SILHS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably use /= instead of .ne. to adhere to the CAM coding standards (it is a should and not a must)

@Katetc
Copy link
Collaborator Author

Katetc commented Jul 29, 2024

One minor tweak requested, but I am approving

I had a minor bug fix too, so I fixed this "if" statement as well.

@cacraigucar cacraigucar removed the request for review from nusbaume July 29, 2024 23:25
@cacraigucar cacraigucar changed the title New CLUBB External to fix GPU problem cam6_4_019: New CLUBB External to fix GPU problem Jul 29, 2024
@Katetc Katetc merged commit eb27509 into ESCOMP:cam_development Aug 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

6 participants