Skip to content

Remove incorrect single_column logic#679

Open
billsacks wants to merge 1 commit into
ESCOMP:mainfrom
billsacks:remove_single_column_check
Open

Remove incorrect single_column logic#679
billsacks wants to merge 1 commit into
ESCOMP:mainfrom
billsacks:remove_single_column_check

Conversation

@billsacks

Copy link
Copy Markdown
Member

Description of changes

This logic was incorrect, I think leading to mesh_mask sometimes being set to null when it shouldn't have been.

Based on Mariana's analysis of the CESM code, it seems like this didn't cause any incorrect behavior, but it was confusing and unnecessary. So we decided to simply remove this unnecessary logic.

Resolves #676

Specific notes

Contributors other than yourself, if any: @mvertens

CMEPS Issues Fixed (include github issue #):

Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial) bfb

Any User Interface Changes (namelist or namelist defaults changes)? mesh_mask is left at its original value rather than being set to null for single point / single-column cases

Testing performed

Ran testing on these changes along with CDEPS changes to fix single-column logic (ESCOMP/CDEPS#422). The following test results describe the results from testing these two changes together; note that the changes in dimension sizes referenced below come from the CDEPS changes.

Ran the following, all with baseline comparisons:

(1) aux_cdeps

(2) aux_cime_baselines

(3) The two tests from the test_scam test list (but moved to derecho):

SCT_D_Ln7.ne3_ne3_mg37.QPC5.derecho_intel.cam-scm_prep
SCT_D_Ln7.ne3_ne3_mg37.QPC6.derecho_intel.cam-scm_prep_c6

(4) A G compset test (since G compsets weren't covered in the other testing):

SMS_D.TL319_t232.G_JRA_RYF.derecho_intel

(5) A selection of single-point tests from aux_clm - the derecho tests suggested in #676 (comment) (not the izumi tests from there, though):

ERS_D_Ld5_Mmpi-serial.1x1_mexicocityMEX.I1PtClm60SpRs.derecho_gnu.clm-CLM1PTStartDate
ERS_D_Mmpi-serial_Ld5.1x1_brazil.I2000Clm50FatesRs.derecho_gnu.clm-FatesCold
ERS_Ly3_Mmpi-serial.1x1_smallvilleIA.IHistClm50BgcCropQianRs.derecho_gnu.clm-cropMonthOutput
ERS_Ly5_Mmpi-serial.1x1_smallvilleIA.I1850Clm50BgcCrop.derecho_gnu.clm-ciso_monthly
ERS_Ly6_Mmpi-serial.1x1_smallvilleIA.IHistClm50BgcCropQianRs.derecho_intel.clm-cropMonthOutput
SMS_D_Ld1_Mmpi-serial.ne3_ne3_mg37.I2000Clm50SpRs.derecho_gnu.clm-ptsRLA
SMS_D_Ld1_Mmpi-serial.ne3_ne3_mg37.I2000Clm50SpRs.derecho_gnu.clm-ptsROA
SMS_D_Ld1_Mmpi-serial.ne3_ne3_mg37.I2000Clm50SpRs.derecho_intel.clm-ptsRLA
SMS_D_Ly6_Mmpi-serial.1x1_smallvilleIA.IHistClm45BgcCropQianRs.derecho_intel.clm-cropMonthOutput
SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Bgc.derecho_gnu.clm-default--clm-NEON-HARV
SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_intel.clm-FatesFireLightningPopDens--clm-NEON-FATES-NIWO
SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60SpRs.derecho_intel.clm-default--clm-NEON-TOOL--clm-nofireemis
SMS_Lm3_D_Mmpi-serial.1x1_brazil.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdHydro
SMS_Ly3_Mmpi-serial.1x1_numaIA.I2000Clm50BgcDvCropQianRs.derecho_gnu.clm-ignor_warn_cropMonthOutputColdStart
SSPMATRIXCN_Ly5_Mmpi-serial.1x1_numaIA.I2000Clm60BgcCropQianRs.derecho_intel.clm-ciso_monthly
SUBSETDATAPOINT_Ld5_D_Mmpi-serial.CLM_USRDAT.I2000Clm60BgcCropCrujra.derecho_intel.clm-default
SUBSETDATAREGION_Ld5_D_Mmpi-serial.CLM_USRDAT.I2000Clm60BgcCropCrujra.derecho_intel.clm-default

All tests passed (other than expected failures) and were bit-for-bit. SMS_Ln9_P1.T42_T42.2000_DATM%QIA_SLND_SICE_DOCN%DOM_SROF_SGLC_SWAV.derecho_intel.datm-scam has different dimension sizes in the cpl hist file (as expected), but when I used ncks to extract just point 1 from the baseline cpl hist file, it's bit-for-bit.

For the two SCP tests, in the baseline, I subset the ocn dimensions to take just the first point (keeping the atm dimensions as is because there is apparently still an issue in nx/ny on the atm side when running with CAM), and then compared the atm & cpl files from case2 against baseline (since these aren't normally compared, and these are the important single-column files); these were bit-for-bit.

Differences in NLCOMP (all expected):

  • I compset 1x1 tests now have mesh_mask UNSET instead of null
  • I1Pt CLM_USRDAT tests now have mesh_mask UNSET instead of null
  • I compset pts tests now have mesh_mask set to a file instead of null; datm model_maskfile and model_meshfile null instead of set, and nx_global and ny_global 1 instead of 0
  • I compset SUBSETDATAPOINT CLM_USRDAT has mesh_mask UNSET instead of null (no substantive diffs in SUBSETDATAREGION namelists)
  • SMS_Ld5_P1.1x1_mexicocityMEX.2000_DATM%1PT_SLND_SICE_SOCN_SROF_SGLC_SWAV_SESP.derecho_intel.datm-1PT has mesh_mask UNSET instead of null
  • SMS_Ln9_P1.T42_T42.2000_DATM%QIA_SLND_SICE_DOCN%DOM_SROF_SGLC_SWAV.derecho_intel.datm-scam has mesh_mask set to a file rather than null; and for both docn_nml and datm_nml, model_maskfile and model_meshfile are now null rather than set, and nx_global and ny_global are 1 rather than 128/64
  • SCT_D_Ln7.ne3_ne3_mg37.QPC6.derecho_intel.cam-scm_prep_c6 and SCT_D_Ln7.ne3_ne3_mg37.QPC5.derecho_intel.cam-scm_prep: case2 has diffs: mesh_mask now set to a file instead of null; docn_in has model_meshfile null instnead of a file, and nx_global 1 instead of 488

Hashes used for testing: ctsm5.4.047 for the selected tests from aux_clm; cesm3_0_alpha09d for others

This logic was incorrect, I think leading to mesh_mask sometimes being
set to null when it shouldn't have been.

Based on Mariana's analysis of the CESM code, it seems like this didn't
cause any incorrect behavior, but it was confusing and unnecessary. So
we decided to simply remove this unnecessary logic.

Resolves ESCOMP#676
@billsacks

Copy link
Copy Markdown
Member Author

@ekluzek and @jtruesdal - I have run pretty extensive testing on this (see testing description in the top-level comment) so feel pretty good about it, but let me know if you want to review this or do some additional testing before it's merged. In particular, the only single-column tests I could find with CAM were:

SCT_D_Ln7.ne3_ne3_mg37.QPC5.derecho_intel.cam-scm_prep
SCT_D_Ln7.ne3_ne3_mg37.QPC6.derecho_intel.cam-scm_prep_c6

@jtruesdal - should any other testing be run on CAM single-column?

@jtruesdal

Copy link
Copy Markdown
Contributor

@billsacks running IOP testing as well as a bfb coupled test with land this afternoon for CMEPS and CDEPS changes.

@ekluzek ekluzek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This just removes the uneeded references to single_column, and the testing shows that it works fine. So approving.

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.

single_column logic in buildnml is incorrect

3 participants