ESCOMP / CMEPS

NUOPC Community Mediator for Earth Prediction Systems
https://escomp.github.io/CMEPS/
24 stars 79 forks source link

Remove negative runoff #471

Closed billsacks closed 5 months ago

billsacks commented 5 months ago

Description of changes

Remove negative runoff before mapping rof -> ocn by setting negative runoff to 0 and downweighting all positive runoff appropriately so that the global runoff sum is the same as before.

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) : Yes - changes answers substantially for runoff fields in B compsets (or other configurations with both active land/river and active ocean) (appears not to change answers in most / all other configurations)

Any User Interface Changes (namelist or namelist defaults changes)? New namelist item: remove_negative_runoff (true by default)

Testing performed

Tested in the context of cesm2_3_beta17

Ran a number of tests using compset 1850_CAM60_CLM50%BGC-CROP_CICE_POP2%ECO_MOSART_SGLC_WW3_SESP_BGC%BDRD and res f19_g17.

Also ran aux_cmeps test suite on derecho; all tests passed and were bit-for-bit except for tests that also failed in the baseline:

    FAIL ERS_Ld5.T62_g17.CMOM.derecho_gnu RUN time=17

    FAIL ERR_Ld5.f09_t061.B1850MOM.derecho_intel.allactive-defaultio SHAREDLIB_BUILD failed to initialize
    FAIL ERS_Ld5.f19_g17.I2000Clm51Bgc.derecho_intel.clm-default CREATE_NEWCASE
    FAIL ERS_Ld5.ne30pg3_t232.BLT1850_v0c.derecho_intel.allactive-defaultio RUN time=729

    FAIL ERP_Ln9.f09_f09_mg17.F2000climo.derecho_nvhpc.cam-outfrq9s MODEL_BUILD time=388
    FAIL ERS_Ld5.T62_g17.GMOM_JRA.derecho_nvhpc MODEL_BUILD time=559

I think the passing tests were bit-for-bit because they didn't include any B compset tests, and it appears that runoff is non-negative in the tests that use drof.

I also ran one nag test on izumi, which also passed and was bit-for-bit: SMS_D_Ld5_P32.f10_f10_mg37.I2000Clm50BgcCropRtm.izumi_nag.rtm-default (I didn't run the full aux_cmeps test suite on izumi, because I noticed that most of the izumi aux_cmeps tests failed in the baseline).

billsacks commented 5 months ago

@jedwards4b I'm assigning you as a reviewer mainly so you can help with the timing of bringing this in, since it will change answers. I expect @mvertens to do the actual code review.

billsacks commented 5 months ago

(To be clear on my last comment: @jedwards4b , I welcome a review from you if you want, but since I have been talking with @mvertens about this, I figured I'd save your time and just get a code review from her.)

billsacks commented 5 months ago

A few specific questions / comments:

(1) Is it a problem that this will change behavior by default for all configurations? We can change the default of the new namelist flag if needed to avoid these answer changes.

(2) Based on some spot-checks from a one-year run of compset 1850_CAM60_CLM50%BGC-CROP_CICE_POP2%ECO_MOSART_SGLC_WW3_SESP_BGC%BDRD at f19_g17:

billsacks commented 5 months ago

I have run the aux_cmeps test suite on derecho with these changes; tests pass and are bit-for-bit. I think this will only change answers in B compsets (or other configurations with both active land/river and active ocean). I have updated the top-level comment accordingly.

I just updated to the latest CMEPS that includes the new glc runoff fields and ran test SMS_D_Ld5.f19_g17.B1850G.derecho_intel.allactive-cism-test_coupling, which passed though changed answers as expected. I confirmed that all four runoff fields are now processed in the new routine that removes negative runoff (by running with the dbug_flag set to 21 to trigger the output).