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

Avoid double accounting of N in senescence #8985

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

peter-devoil
Copy link
Contributor

working on #8917

@@ -891,13 +891,13 @@ private void ApplySenescence()
if (!MathUtilities.IsPositive(dltSenescedBiomass)) return;

double slnToday = MathUtilities.Divide(Live.N, laiToday, 0.0);
DltSenescedN += DltSenescedLai * Math.Max(slnToday, 0.0);
double dltSenescedN = DltSenescedLai * Math.Max(slnToday, 0.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

From memory, DltSenescedN is used as a reporting/tracking variable. It can also be senesced when providing N to another organ and is therefore cumulative. I should be confident that we weren't double counting it as it mirrored the outputs from Classic that were much easier to follow - but it's been a while since I was in that code.
The outcome here shouldn't be an exception though - it should set the senescence to the live amount and then kill the plant - normally the reduction in LAI would catch this, but I'm guessing that the amounts that we're talking about are very small. There is an issue with how the LAI checks work due to frost removing LAI but not killing the plant completely. There's probably also a case to be made about going below the structural minimum %?

@par456
Copy link
Collaborator

par456 commented May 27, 2024

retest this please jenkins

@hol353
Copy link
Contributor

hol353 commented Jul 30, 2024

@jbrider @peter-devoil The stats have changed for this one - mostly improved. Are you happy for this to be merged?

@peter-devoil
Copy link
Contributor Author

I'll work it through with jason. From what I can tell, the offender is being called twice/day when it should be once.

@peter-devoil peter-devoil deleted the sorgNSenescence branch September 2, 2024 01:32
@peter-devoil
Copy link
Contributor Author

@jbrider, I've had another look at this and suspect it is double accounting. The "double calling" isn't happening.

It is set first in ProvideNThroughSenescence() which calculates the N lost in senesced material in differing ways pre/post flowering

It's again incremented in ApplySenescence(), which is using the same DltSenescedLai calculated in ProvideNThroughSenescence() earlier

I think the test, and second increment is redundant.

@peter-devoil
Copy link
Contributor Author

undo

@peter-devoil peter-devoil reopened this Sep 2, 2024
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.

4 participants