-
Notifications
You must be signed in to change notification settings - Fork 142
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_3_148: Provide RRTMGP as a radiation parameterization #909
Conversation
@cacraigucar there are a number of ways to limit the checked out repository in git, specifically
git submodule add also has a --depth option. However I don't think that there is an easy way to apply them with manage_externals. |
Doing a clone by hand with --depth 1 doesn't appear to impact the size at all. The .git/objects/pack directory is still the same size. Any other suggestions? |
Actually you can bring these in as submodules using the --depth option as suggested and then use the manage_externals interface to submodules:
|
Perhaps you made an error?
git clone -b v1.7 https://github.com/earth-system-radiation/rte-rrtmgp.git rte-rrtmgp.full You are in 'detached HEAD' state. You can look around, make experimental If you want to create a new branch to retain commits you create, you may git switch -c Or undo this operation with: git switch - Turn off this advice by setting config variable advice.detachedHead to false casper-login1: ~/tmp
|
Yup - I must have done something wrong, as my size is dramtically reduced. What exactly do I put in the Externals.cfg file?
Yup, I must have done something wrong as I now see the size I need. Sorry about that! What exactly do we need to put into our workflow and/or manage_externals? RRTMGP in this PR is a part of Externals_CAM.cfg. When I add "from_submodule = True" into the rrtmgp sections, I get an error from manage_externals that I have over specified with both from_submodule and tag. I also don't know where I'd put in the --depth 1. I'm trying to find a solution that a user cloning CAM and running manage_externals will get the smaller sized checkout. Is this possible? |
You want to bring rrtmgp into cam as a submodule |
Thanks @jedwards4b . I think I finally understand (I've never dealt with submodules before). @brian-eaton - Can you add the two rrtmgp checkouts as submodules (using the command Jim provided above with the --depth 1)? Then you'll need to modify the Externals_CAM.cfg file for these two externals. This should reduce the RRTMGP size tremendously. I've done this by hand and it appears to work for me, so let me know if you have any questions. |
Just so you are aware for developers - you can go to the submodule directory and run git fetch origin to get the entire repository locally if you want it. |
@@ -1221,7 +1242,7 @@ if ($carma eq 'bc_strat') { | |||
} | |||
} | |||
|
|||
if ($rrtmg) { | |||
if ($rad_pkg eq 'rrtmg') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving this comment un-resolved so that we remember to make a Github issue about CARMA not working with RRTMGP after this PR has been merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for responding to all of my comments! This PR looks good to me now. I did find one last set of typos, and one optional suggestion, but I don't believe it needs another review from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting my comments from my short review (lots still to go).
[rte-rrtmgp] | ||
local_path = src/physics/rrtmgp/ext | ||
protocol = git | ||
repo_url = https://github.com/earth-system-radiation/rte-rrtmgp.git | ||
tag = v1.7 | ||
required = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change these once rrtmgp is brought in as a submodule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per comments below (thanks @jedwards4b and @nusbaume) will leave this alone for now. Will revisit when we bring in git-fleximod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next round of comments - still need to look at a few more routines
@jedwards4b, I'm trying to add the RRTMGP source as a submodule following your instructions, but am getting an error. Here's what happens, starting from a clean clone of my CAM fork on branch rrtmgp:
So I added the -f option:
I don't understand this error. |
Hi @brian-eaton @jedwards4b @cacraigucar I have been working on this today, and I have reached a point where I believe there is a bug in manage_externals which prevents submodules from working properly. When I add the two RRTMGP repos as submodules (which I know is working because I can use commands like
The problem is that the If I am wrong here and someone else can get it to work properly then definitely ignore this comment post. However if everyone else runs into the same problem then at this point I might vote to just leave the checkout as-is, and then once we bring in git-fleximod we'll implement a single depth checkout to reduce the large disk space overhead. To me that seems like a better use of everyone's time then trying to fix software (manage_externals) which is going away soon. Thoughts? |
@nusbaume is correct, this is a manage_extenals bug - if you checkout cesm so that cam is not the toplevel directory the submodule checkout will work correctly. @brian-eaton Reverse the --depth and -b options: git submodule add -b v1.7 --depth=1 -f https://github.com/earth-system-radiation/rte-rrtmgp.git src/physics/rrtmgp/ext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final set of review comments.
Kudos to whoever wrote the excellent code comments (especially in the rrtmgp_inputs.F90 module). They really helped with reviewing the code!
! Mapping from RRTMG shortwave bands to RRTMGP. Currently needed to continue using | ||
! the SW optics datasets from RRTMG (even thought there is a slight mismatch in the | ||
! band boundaries of the 2 bands that overlap with the LW bands). | ||
integer, parameter, dimension(14) :: rrtmg_to_rrtmgp_swbands = & | ||
[ 14, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a check to make sure that the bands in RRTMGP have not changed position (assuming this is a list of index locations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the RRTMG code, the RRTMGP interface code doesn't make any assumption about the spectral band boundaries or their order. That data is read from input datasets. What the RRTMGP code is assuming is the band order in the RRTMG optics datasets. The reason this mapping is hard-coded is that the optics datasets don't contain the metadata that they should contain to describe the spectral bands. Furthermore a couple of the bands used by RRTMGP don't have the same bounds that the RRTMG versions have. The bottom line is that the optics datasets used by RRTMGP should be updated to match the RRTMGP band boundaries. But this hack enables continued use of the RRTMG datasets until the scientists decide the approximation involved is no longer acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this comment "unresolved" as it will make it easier to find this explanation.
! Add extra layer values if needed. | ||
if (nlay == pverp) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work with all variations of CAM current and future since model tops can change? I would think a better check would be to retrieve the pressure at the top and make the if check based off of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nlay
depends on the number of model layers that are below the RRTMGP top of 1 Pa. It is set in subroutine radiation_init
making use of the reference pressures at the model layer interfaces. The condition nlay == pverp
tells you that the radiation is using 1 more than the pver
layers that CAM has, hence the addition of the "extra layer". Dealing with the fact that RRTMGP usually uses a different number of layers than CAM is the most subtle aspect of the interface code. But this is also true for RRTMG, so we were able to leverage a lot of existing code to deal with this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I am leaving this unresolved so we can easily see your explanation about this important aspect of the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job everyone on this PR!!!
! Add extra layer values if needed. | ||
if (nlay == pverp) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I am leaving this unresolved so we can easily see your explanation about this important aspect of the code
! Mapping from RRTMG shortwave bands to RRTMGP. Currently needed to continue using | ||
! the SW optics datasets from RRTMG (even thought there is a slight mismatch in the | ||
! band boundaries of the 2 bands that overlap with the LW bands). | ||
integer, parameter, dimension(14) :: rrtmg_to_rrtmgp_swbands = & | ||
[ 14, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this comment "unresolved" as it will make it easier to find this explanation.
<test compset="QPC6" grid="ne5pg3_ne5pg3_mg37" name="ERP_D_Ln9" testmods="cam/outfrq9s_rrtmgp"> | ||
<machines> | ||
<machine name="izumi" compiler="gnu" category="aux_cam"/> | ||
</machines> | ||
<options> | ||
<option name="wallclock">00:10:00</option> | ||
</options> | ||
</test> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to keep this in mind if more tests start to use RRTMGP (or if it ever becomes default).
Merge pull request ESCOMP#909 from brian-eaton/rrtmgp
ESCOMP/CAM PR ESCOMP#909 from brian-eaton/rrtmgp. Provide RRTMGP as a radiation parameterization.
ESCOMP/CAM PR ESCOMP#909 from brian-eaton/rrtmgp. Provide RRTMGP as a radiation parameterization.
Merge pull request ESCOMP#909 from brian-eaton/rrtmgp Provide RRTMGP as a radiation parameterization ESCOMP commit: 660e3fd
Issue #255 - Provide RRTMGP as a radiation parameterization
resolves #255
Misc:
. The use case file
1850_cam5.xml
is returned to the source code. This is to facilitate running the F1850 compset with CAM5 physics as discussed in issue #393.