-
Notifications
You must be signed in to change notification settings - Fork 167
Correct allocation and assignment error for 'hgt_matrix_half' field in cospsimulator_intr.F90 #1355
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
base: cam_development
Are you sure you want to change the base?
Conversation
| cospstateIN%phalf = state%pint(:ncol,ktop:pverp) | ||
| cospstateIN%hgt_matrix = zmid | ||
| cospstateIN%hgt_matrix_half = zint | ||
| cospstateIN%hgt_matrix_half = zint(1:ncol,2:nlayp) ! COSP wants half levels without model top |
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.
This is very confusing. It appears that cosp_change_vertical_grid does want the half-level heights at the bottom of each layer. But there is a lot of code still that assumes hgt_matrix_half is dimensioned (npoints,nlevels+1). For example in function COSP_SIMULATOR we find
rttovIN%h_surf => cospgridIN%hgt_matrix_half(:,cospIN%Nlevels+1)
And in subroutine lidar_column we find
real(wp),intent(in),dimension(npoints,nlevels+1) :: &
zlev_half ! Model half levels
I think having hgt_matrix_half only contain the bottom half levels is confusing because you don't know without searching code whether the array contains the top or the bottom levels. It's also confusing because it's unexpected. I expect the heights in hgt_matrix_half to correspond to the pressures in phalf.
My suggestion is that hgt_matrix_half be left as it is, and that the calls to cosp_change_vertical_grid be modified to pass the correct section containing the bottom half levels, e.g.,
call cosp_change_vertical_grid(cloudsatIN%Npoints,1,cloudsatIN%Nlevels, &
cospgridIN%hgt_matrix(:,cloudsatIN%Nlevels:1:-1), &
cospgridIN%hgt_matrix_half(:,cloudsatIN%Nlevels+1:2:-1),betamol_in, &
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.
@brian-eaton thanks for digging into this! I had been comparing with the current COSP version (https://github.com/CFMIP/COSPv2.0) rather than the tag that is currently used in CESM2. hgt_matrix_half is consistently dimensioned (npoints,nlevels+1) there and the calls to lidar_column and cosp_change_vertical_grid do not use the height at the top of the model.
So it seems that the changes I requested require a more recent version of COSP. @cacraigucar does it then make the most sense to wait to merge this PR until we have an official COSP tag with pending updates that do not involve RTTOV? Then this PR can include a change in the git-fleximod settings to point to an appropriate COSP tag that works with these changes. So next steps would be:
- Merge pending changes to COSP that do not involve RTTOV. Produce a tag for CESM.
- Point to that tag in this PR, merge.
- Produce regression test values.
- Test COSP-RTTOV PR and evaluate against regression tests.
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.
Yes, that sounds like a plan. We will need to proceed cautiously before we move forward with step 2-3 where we commit the new COSP (with the bug fix). It probably will need to have some testing by @cecilehannay before we bring it in. COSP is used at times for the CESM3 science runs, and we need to make sure it is working properly before we commit it.
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.
Hi @jshaw35. I've taken a look at the current head of the master branch of COSPv2.0 and see that the dimension of hgt_matrix_half has been changed to (npoints,nlevels) and that it now contains just the bottom interfaces. I agree that the path forward is to bring in an updated COSP tag which will require the changes in this PR along with appropriate changes to how hgt_matrix_half is initialized from the local variable zint.
Note also that cospsimulator_intr.F90 has an option to output the variables that are COSP inputs to the cosp_histfile_aux stream. The addfld call for ZLEV_HALF_COSP needs the vertical dimension updated.
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.
Note for self. Relevant changes between the CESM tag and the master branch of COSP are here:
CFMIP/COSPv2.0@master...CESM_v2.1.4
CFMIP/COSPv2.0@CESM_v2.1.4...CFMIP:COSPv2.0:master
I will need to update the cospsimulator_intr.F90 to allocate and assign both hgt_matrix_half and phalf without the model top level when we use a more current tag of COSP.
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.
I agree that bringing in the updated version of COSP is the right move. I think it will also allow us to add some additional diagnostics (which I will try to look into separately).
Thanks @jshaw35 and @brian-eaton for moving this forward.
|
@jshaw35 - Just a reminder that as soon as you add the COSP external update (and you have tested it with your mods), we can have the reviewers test and approve this PR and we can move it forward. |
|
@cacraigucar I have just updated .gitmodules to reference the new COSP tag (most current COSP with no RTTOV). We will need to update the COSP driver following the discussion above. |
@jshaw35 I assume that you will be making the change required? |
@cacraigucar yes! I don't expect it to take too long but I am sure yet when I will have the time. Hopefully sooner than later but almost certainly by September 15. @brian-eaton the new tag for COSP should now align with the changes in this PR, resolving your points. I will review any additional changes needed to the driver since the cosp2.1.4 tag and then request an additional review. |
|
@jshaw35 -- If you have a chance, could you also see if the MODIS cloud phase diagnostics are included in the updated version of COSP? I think they should be, but we might need to add them to the CAM outputs. See, e.g., Duran et al. 2025. |
@brianpm , yes I don't think this should be a problem. I've just put in the new warm rain diagnostics and can add the joint histograms you're requesting. To be clear, you want the "modis_LWP_vs_ReffLIQ" and "modis_IWP_vs_ReffICE" variables, anything else? |
|
@brianpm I've just added the MODIS LWP-REFF histograms in addition to the warm rain diagnostics from Michibata (2019). The fields are not modified by the CESM coupler (besides separating variables, naming, etc) and I've tried to follow existing naming conventions and supply units/bounds appropriately. Would you mind reviewing the output? Average fields from a month of CAM can be found here: /glade/work/jonahshaw/COSP_RTTOV_files/COSP2.1.8_tests/cosp2.1.8_newvars.nc |
|
Thanks @jshaw35 ! Looking at the MODIS simulator, it looks like they have histograms of:
I think we should add all of them if we can. What do you think? |
@brianpm easy to do it now! I believe those additional fields are internally named in COSP as: If this looks right to you I will go ahead and add the last two histograms. Any preferences on how to name the last two fields? |
|
Yeah, that's exactly what I was looking at! Thanks!! |
|
@brianpm all MODIS cloud histograms should now be available as output
Would you mind reviewing the outputs? Average fields from a month of CAM can be found here: /glade/work/jonahshaw/COSP_RTTOV_files/COSP2.1.8_tests/cosp2.1.8_newvars2.nc |
|
@jshaw35 -- Looking through your file, two small things. First I think the units for the coordinate variables might be wrong:
Second, I don't see CLRIMODIS and CLRLMODIS in the file. |
|
@brianpm thanks for reviewing! Comments in-line below.
Yep, you're totally right here. Now that I think about it, this has been in the CESM code for years.
Yep again!
These are actually correct. The upper bound on the LWP and IWP is 20 kg m-2 for both liquid and ice, so the bin center appears to be abnormally high. I think this is just meant to include all large values. The bounds arrays from cosp_config.F90 make things a bit clearer:
I've updated the file with them now from the same run (so units will still be off). |
|
@cacraigucar @brianpm @brian-eaton I think this PR is ready for review unless their are additional outstanding issues. |
| call addfld('CS_WR_CFODD_REFF_SMALL',(/'cosp_cfodd_dbze','cosp_cfodd_icod'/), 'A', 'fraction', & | ||
| '# of CFODD (05 < Reff < 12 micron)', flag_xyfill=.true., fill_value=R_UNDEF) | ||
| call addfld('CS_WR_CFODD_REFF_MEDIUM',(/'cosp_cfodd_dbze','cosp_cfodd_icod'/), 'A', 'fraction', & | ||
| '# of CFODD (12 < Reff < 18 micron)', flag_xyfill=.true., fill_value=R_UNDEF) | ||
| call addfld('CS_WR_CFODD_REFF_LARGE',(/'cosp_cfodd_dbze','cosp_cfodd_icod'/), 'A', 'fraction', & | ||
| '# of CFODD (18 < Reff < 35 micron)', flag_xyfill=.true., fill_value=R_UNDEF) |
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.
'fraction' is not a valid UDUNIT. Should be '1'.
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.
done
Merge pull request ESCOMP#1361 from jimmielin/hplin/hb_diff
Fixes for input interpolation routines (aircraft runs only)
Fix frontogenesis function bug and change CISM2 to SGLC in FCHIST_GC compset
… to avoid dangling pointer error in NAG.
|
@cacraigucar I think I may have identified and corrected the NAG error (due to pulling in the arrays defining the bin centers for the new dimensions and passing them into add_hist_coord without explicitly defining them as targets), would you mind testing on izumi? |
.gitmodules
Outdated
| url = https://github.com/CFMIP/COSPv2.0/releases/tag/v2.1.9 | ||
| fxrequired = AlwaysRequired | ||
| fxsparse = ../.cosp_sparse_checkout | ||
| fxtag = v2.1.4cesm | ||
| fxtag = v2.1.8 |
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.
When doing a fresh clone using this .gitmodules file, git-fleximod indicates an error occurred with the cosp checkout. Two items to note:
1: Typically we have the url just point to the github repo and the tag is set in fxtag. (Note I tried setting the url back to https://github.com/CFMIP/COSPv2.0 and using either tag v2.1.9 or releases/tag/v2.1.9 and neither worked)
2: The old tag was a cesm specific tag. Do we need a new cesm specific tag?
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.
- I have made changes to git-fleximod that allow me to run ./bin/git-fleximod update successfully when I clone the PR into Derecho (CAM only). The changes to git-fleximod are AI-produced and should be reviewed.
- No, this PR resolves the inconsistencies between the hgt_matrix_half fields that led to a CESM specific COSP2 tag.
|
You would need to redirect this to the correct repository, but I think that |
|
@jedwards4b if I just want to update the git-fleximod version in CAM how should I proceed? I see two versions of git-fleximod in CAM. I believe that the first (and older) version is used when I call "./bin/git-fleximod update" because modifying that source code allowed me to bring in the new COSP2 tag without errors. Does this mean that the older version is needed for some operations and the newer version is used elsewhere? Some guidance on this code structure would be helpful, I don't want to break other functionality by updating the first git-fleximod path. .lib/git-fleximod/git_fleximod/ (0.9.4) |
|
You are correct, there is a problem in the way that git-fleximod was updated and cam is still using an older version. I have opened issue #1421 to address this. Thanks |
|
I have confirmed that with the PR #1422 |
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.
Changes to git-fleximod should be removed and this PR should be merged after #1422
…ted CAM development branch
Closes #1354
@cacraigucar