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

9434 Accumulate Function Resetting at Harvest #9496

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

Conversation

par456
Copy link
Collaborator

@par456 par456 commented Dec 4, 2024

Resovles #9434

@par456
Copy link
Collaborator Author

par456 commented Dec 4, 2024

retest this please jenkins

@par456 par456 requested review from ric394 and removed request for ric394 December 5, 2024 01:21
@par456
Copy link
Collaborator Author

par456 commented Dec 5, 2024

So the solution I've written up here is to add a "PostHarvesting" event (using the naming convention that "PostPhenology" has) which is called after the "Harvesting" event when reporting happens.

The PostHarvesting event can then be listened for by organs which have a biomass removal step that happens at harvest, but after the Harvesting step (previously this was done in a loop in the Plant Harvest function where each organ's harvest function was called). I've then also attached the Accumulators to PostHarvesting for their removal step, which previously would happen at the same time as reporting (which lead to inconsistent results due to event timing).

Only question I have is if we keep the event as "PostHarvesting" or if a better name would be good here. Considering all these are biomass removal functions, maybe something like "BiomassRemovalAfterHarvesting" might be a better name if we wanted something more descriptive.

Otherwise, this PR should be good to go. No changes in stats and nothing broke with it.

@par456 par456 requested a review from hol353 December 5, 2024 01:31
@BrianCollinss
Copy link
Member

Can they simply be named PreHarvest and PostHarvest?

@par456
Copy link
Collaborator Author

par456 commented Dec 6, 2024

I wouldn't change the name of the Harvesting event as that is used by everyone in their report frequencies, so it would be a difficult change for everyone to stop using Harvesting and starting using PreHarvest, which also doesn't make as much sense in the reporting context.

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.

2 participants