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

Fixed conversion of minimal CAPAC to RUNOFF (catchment.F90) #991

Merged

Conversation

gmao-rreichle
Copy link
Contributor

Addresses #990

Bug fix: Previously, minimal CAPAC values were converted into runoff by adding them directly into total RUNOFF, but not into one of its components (RUNSRF or BFLOW), which violated the definition of total RUNOFF (=RUNSRF + BFLOW). This fix adds the minimal CAPAC into RUNSRF and thus preserves the equation of total RUNOFF.

Changes should be 0-diff for restarts in AGCM and LDAS, and differences should only be seen in the RUNSRF and possibly RUNOFF diagnostics. The current implementation of the bug fix prioritizes efficiency and logic but may result in (minimal) changes in the total RUNOFF diagnostic, which may in turn result in non-0-diff changes in the coupled model (AOGCM). If needed, we could change the implementation of the bug fix to also make it 0-diff for coupled model restarts.

@biljanaorescanin: When you get a chance, please test this PR for the GCM (incl. the coupled model) and the LDAS.

cc: @rdkoster

Bug fix: Previously, minimal CAPAC values were converted into runoff by adding them directly into total RUNOFF, but not into one of its components (RUNSRF or BFLOW), which violated the definition of total RUNOFF (=RUNSRF + BFLOW).  This fix adds the minimal CAPAC into RUNSRF and thus preserves the equation of total RUNOFF.
@gmao-rreichle gmao-rreichle added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. Non 0-diff The changes in this pull request are non-zero-diff bugfix This fixes a bug labels Sep 3, 2024
Bug fix (CatchCN): Previously, minimal CAPAC values were converted into runoff by adding them directly into total RUNOFF, but not into one of its components (RUNSRF or BFLOW), which violated the definition of total RUNOFF (=RUNSRF + BFLOW).  This fix adds the minimal CAPAC into RUNSRF and thus preserves the equation of total RUNOFF.
@biljanaorescanin
Copy link
Contributor

Testing summary:

Difference is always only in variable RUNOFF and it is roundoff. Restart files are zero diff in all the experiments.

1. AMIP run
Surf collection will fail for example for one time step:
Comparing geosgcm_surf.20000414_2230z...
Failure!
Checking for data differences
Variable Group Count Sum AbsSum Min Max Range Mean StdDev
RUNOFF / 323 -3.98556e-14 3.98556e-14 -6.39766e-15 -3.42058e-36 6.39766e-15 -1.23392e-16 5.98459e-16

or diff view for few differences:
DIFFER : VARIABLE : RUNOFF : POSITION : [0,65,136] : VALUES : 2.8095e-31 <> 8.52751e-19 : PERCENT : 100
DIFFER : VARIABLE : RUNOFF : POSITION : [0,65,137] : VALUES : 5.78158e-22 <> 8.41819e-18 : PERCENT : 99.9931
DIFFER : VARIABLE : RUNOFF : POSITION : [0,65,138] : VALUES : 3.54139e-20 <> 1.79776e-17 : PERCENT : 99.803

2. Replay run
Surf collection difference for one time step:
Comparing geosgcm_surf.20000414_2230z...
Failure!
Checking for data differences
Variable Group Count Sum AbsSum Min Max Range Mean StdDev
RUNOFF / 326 -6.13807e-14 6.13807e-14 -1.42109e-14 -3.0702e-36 1.42109e-14 -1.88284e-16 1.12082e-15

3. Replay No-Inc run
Comparing geosgcm_surf.20000414_2230z...
Failure!
Checking for data differences
Variable Group Count Sum AbsSum Min Max Range Mean StdDev
RUNOFF / 323 -3.98556e-14 3.98556e-14 -6.39766e-15 -3.42058e-36 6.39766e-15 -1.23392e-16 5.98459e-16
Checking for metadata differences

4. MOM6 Coupled run

All comparisons are zero diff.

5. GEOSldas regression tests

All tests fail for comparison in LND collection for model runs and GPH collection for assim runs.
Roundoff in both cases for variable RUNOFF for model runs and variable overland_runoff_flux for assim runs.

@gmao-rreichle gmao-rreichle marked this pull request as ready for review September 6, 2024 13:04
@gmao-rreichle gmao-rreichle requested a review from a team as a code owner September 6, 2024 13:04
@gmao-rreichle
Copy link
Contributor Author

@sdrabenh: This PR is ready for merging. I apologize for what looks like confusing labels. Would it make sense to introduce one or two more labels: 0-diff for restarts and/or non-0-diff for diagnostics? I'm never quite sure what the expectation is when we say it's 0-diff without any qualifier.

@sdrabenh sdrabenh merged commit 24de04c into develop Sep 13, 2024
9 checks passed
@sdrabenh sdrabenh deleted the bugfix/rreichle/conversion_of_minimal_CAPAC_to_RUNOFF branch September 13, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch. bugfix This fixes a bug Non 0-diff The changes in this pull request are non-zero-diff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants