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

hydro fix and simple fixes #955

Merged
merged 3 commits into from
Dec 1, 2022
Merged

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Nov 30, 2022

Description:

This is a bug fix that adds a missing pointer setting. This also makes some subtle code clean up.

Fixes: #950
Fixes: #954

Collaborators:

Expectation of Answer Changes:

This should not change answers for non-hydro cases. For hydro simulations, this should change answers when using variable rooting depth with plant size. It is possible that differences in hydro simulations will not show up in regression tests.

Checklist:

  • 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:

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

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

FATES baseline hash-tag:

Test Output:

PENDING

@rgknox rgknox assigned rgknox and glemieux and unassigned rgknox Nov 30, 2022
@rgknox rgknox requested a review from JunyanDing November 30, 2022 02:04
Copy link
Contributor

@JunyanDing JunyanDing left a comment

Choose a reason for hiding this comment

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

Looks good!

@rgknox
Copy link
Contributor Author

rgknox commented Nov 30, 2022

Great, I have nothing left to commit to this

@glemieux
Copy link
Contributor

Awesome. I'll run these through the standard tests soon.

@glemieux
Copy link
Contributor

glemieux commented Dec 1, 2022

Regression testing on Cheyenne is complete. All expected tests PASS with the only DIFFs present in the two hydro test cases as expected. Looking at the differences, we see the FATES_ROOTUPTAKE_SL as being the first difference on the initial output tilmestep, which makes sense in the context of this change. The variable immediately affected by the change is rootl_sl which affects the FATES_ROOTUPTAKE_SL just down stream here:

weight_sl(j_bc) = csite_hydr%rootl_sl(j_bc)
else
write(fates_log(),*) 'Unknown rhiz->soil disaggregation method',rootflux_disagg
call endrun(msg=errMsg(sourcefile, __LINE__))
end if
sumweight = sumweight + weight_sl(j_bc)
end do
! Second pass, apply normalized weighting factors for fluxes
do j_bc = j_t,j_b
! Fill the output array to the HLM
bc_out(s)%qflx_soil2root_sisl(j_bc) = qflx_soil2root_rhiz * weight_sl(j_bc)/sumweight
! Save root uptake for history diagnostics [kg/m/s]
csite_hydr%rootuptake_sl(j_bc) = qflx_soil2root_rhiz * weight_sl(j_bc)/sumweight

This is ready to integrate.

Test results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr955

@glemieux glemieux merged commit 474fa84 into NGEET:main Dec 1, 2022
@rgknox rgknox deleted the hydro-simple-fixes branch October 31, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

various fixes from compiler warnings uninitialized hydro data structure
3 participants