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

bug fix: units of RUNSRF in Catchment #535

Merged
merged 6 commits into from
Feb 10, 2022

Conversation

gmao-rreichle
Copy link
Contributor

@gmao-rreichle gmao-rreichle commented Feb 5, 2022

[Edited 6 Feb 2022, ~2:45pm, by Reichle]

  • Fixes units of surface runoff (RUNSRF) in Catchment[CN]
  • Clarifies units of throughfall (THRU[x])
  • Moves rzdrain() to lsm_routines.F90
  • Cosmetic changes (hide whitespace changes when looking at differences)

Problem:

Water fluxes in catchment() are in units of kg m-2 s-1 except RUNSRF:
RUNSRF is in [kg m-2 s-1] into and out of subroutine partition().
Next, RUNSRF is assumed to be in "volume" units [kg m-2] into and out of subroutine rzdrain().
Finally, RUNSRF units going into subroutine srunoff() are [kg m-2] but returned in [kg m-2 s-1].

Also, subroutine interc() has precipitation inputs in flux units [kg m-2 s-1] and returns throughfall in as "volume" [kg m-2].

See attached Word docx for notes.

Also, identical copies of subroutine rzdrain() are included in catchment.F90 and catchmentCN.F90.

WATER_FLUXES_UNITS_IN_CATCHMENT.docx

Solution:

This PR changes the units of RUNSRF for all input and output arguments to be in flux units [kg m-2 s-1], for consistency with the units of all other water fluxes. The calculations within subroutine srunoff() remain in volume units.

The RUNSRF changes are zero-diff (initial tests with GEOSldas passed successfully).

I also tested changing the units of throughfall in the argument lists of interc() and srunoff(), but the result was not 0-diff owing to the additional divisions and multiplications. Rather than deal with a non-zero-diff change, I decided to keep the "volume" units of throughfall and added clarifying comments.

Finally, rzdrain() was moved to lsm_routines.F90. Unfortunately, the move obscures the bug fix needed in rzdrain(). The inline comments below point out the computational changes. All other changes are simply for wiring or cosmetic.

cc: @biljanaorescanin @rdkoster @sdrabenh

@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 Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) bugfix This fixes a bug cleanup labels Feb 5, 2022
@gmao-rreichle gmao-rreichle requested a review from a team as a code owner February 5, 2022 23:20
@gmao-rreichle gmao-rreichle self-assigned this Feb 5, 2022
- clarified units of surface runoff, infiltration, and throughfall
  in subroutines interc(), rzdrain(), srunof()
- moved subroutine rzdrain() to lsm_routines.F90 (identical for
  Catchment and CatchmentCN)
@gmao-rreichle gmao-rreichle removed the Non 0-diff The changes in this pull request are non-zero-diff label Feb 6, 2022
THRUL = THRUL_VOL ! introduced *_VOL variables for clarity
THRUC = THRUC_VOL

RUNSRF = RUNSRF * DTSTEP ! convert input surface runoff from flux to "volume" units
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, the first of two computational changes is here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks correct.

IF(CATDEF(N) .LT. 0.) THEN
! bug fix: RUNSRF in flux units [kg m-2 s-1] for consistency with partition()
! reichle, 6 Feb 2022
RUNSRF(N)=RUNSRF(N)-CATDEF(N)/DTSTEP
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, the second of two computational changes is here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks correct.

@gmao-rreichle gmao-rreichle removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Feb 6, 2022
Copy link
Contributor

@rdkoster rdkoster left a comment

Choose a reason for hiding this comment

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

I agree with the changes in this pull request.

@biljanaorescanin
Copy link
Contributor

biljanaorescanin commented Feb 7, 2022

Tests summary:

  1. GEOSldas tests were 0-diff [except for tests with "aggressive" builds, which is expected and does not​ violate 0-diff; non-zero-diff for "aggressive" tests were limited to roundoff differences for runoff variable in "lnd" history collection for "model" test and overland_runoff_flux variable in "gph" collection for "assim" test ]
  2. AGCM test was 0-diff.
    @gmao-rreichle @sdrabenh

@sdrabenh sdrabenh merged commit 7bae5aa into develop Feb 10, 2022
@sdrabenh sdrabenh deleted the bugfix/rreichle/Catchment_RUNSRF_units branch February 10, 2022 21:32
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 cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants