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

Move all the patch-level hist vars to site-level #642

Merged
merged 5 commits into from
May 13, 2020

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Apr 29, 2020

There have been a few variables that stayed on the old patch native weighting in the history writing. The use of these is confusing for a few reasons: its confusingly inconsistent with how we're doing things otherwise, its more complicated (since the patch weighting system weighted by patch canopy area rather than patch area for reasons having to do with radiative transfer and the existence of a "bare-ground" patch in that scheme), and it won't play well with more complex subgrid structure once we start moving to using other-than-natural-veg land types. So this PR gets rid of all of the remaining ones (those that aren't already removed in #640).

problem noted in #475 and #630. fixes #475.

Collaborators:

discussed with @rgknox, @rosiealice, and @glemieux

Expectation of Answer Changes:

should be bit for bit or roundoff-level changes only, with the exception that I got rid of one redundant variable, NPP_column.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Test Results:

not yet run through test suite.

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

@glemieux glemieux requested a review from rosiealice May 5, 2020 01:18
@glemieux glemieux self-assigned this May 5, 2020
@@ -3132,36 +3104,26 @@ subroutine update_history_prod(this,nc,nsites,sites,dt_tstep)
ccohort => cpatch%shortest
do while(associated(ccohort))

! TODO: we need a standardized logical function on this (used lots, RGK)
Copy link
Contributor

@glemieux glemieux May 6, 2020

Choose a reason for hiding this comment

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

Nice that this update takes care of the TO DO.

Copy link
Contributor

@glemieux glemieux left a comment

Choose a reason for hiding this comment

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

Just a couple of small potential changes.

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
@glemieux
Copy link
Contributor

glemieux commented May 6, 2020

All expected PASS:

/glade/u/home/glemieux/scratch/clmed-tests/pr642-nopa-hist-vars-C242d6d14-Fc65614ee.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/pr642-nopa-hist-vars-C242d6d14-Fc65614ee.fates.cheyenne.gnu

All DIFFs related to the changes from patch to site-level. @ckoven reviewed and thinks the differences are reasonable, noting that it likely has to do with the "time-averaging differences in patch areas".

@ckoven
Copy link
Contributor Author

ckoven commented May 6, 2020

at least i think so...

@rgknox
Copy link
Contributor

rgknox commented May 7, 2020

I like how more lines were removed than were added. I think this looks good.

@rgknox rgknox self-requested a review May 7, 2020 16:36
Deconflicting PR640 fire variable update with PR 642
@glemieux
Copy link
Contributor

Status update: ran a quick test late yesterday after the deconflict merge and a bunch of tests are failing during run time. Looking into the cause.

@glemieux
Copy link
Contributor

glemieux commented May 12, 2020

Problem turned out to be the fact that #640 updated the vname for three of the fire variables (see 5ac1ceb). I assumed the capitalization wouldn't matter and so didn't retest after @ckoven committed the changes. The user namelist variables need to have the same case so the tests where failing when looking for the lowercase spelling. Lesson learned. CTSM PR created to correct this: ESCOMP/CTSM#1011

@glemieux
Copy link
Contributor

Re-test against current master baseline results in the same DIFFs as seen previously. All expected PASS.

/glade/u/home/glemieux/scratch/clmed-tests/sci.1.35.3_api.10.0.1-clm5.0.30-C95df92ee-F8259b5b2.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/sci.1.35.3_api.10.0.1-clm5.0.30-C95df92ee-F8259b5b2.fates.cheyenne.gnu

@glemieux glemieux merged commit b78c500 into NGEET:master May 13, 2020
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.

remove patch level history output?
3 participants