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

Fix N reallocation/retranslocation issue in SimpleLeaf #8726

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

Conversation

BrianCollinss
Copy link
Member

@BrianCollinss BrianCollinss commented Mar 4, 2024

Resolves #8714

Also in Soybean (#8714 (comment)) and Sorghum (#8714 (comment))

There might be a problem in the way Reallocation is calculated. However, based on what there is in the Leaf class, I removed two checks/tests from SimpleLeaf and added one single check similar to the one that exists in the Leaf class.

@hol353
Copy link
Contributor

hol353 commented Mar 4, 2024

@hut104 Can you have a look at this one?

@BrianCollinss
Copy link
Member Author

BrianCollinss commented Mar 5, 2024

Hey @hut104. Eagerly looking forward to your comments! :)

@par456
Copy link
Collaborator

par456 commented Mar 21, 2024

Could you explain why those tests aren't necessary with your changes? It might help us understand what your changes do. You said you changed SimpleLeaf and Leaf, but only one file was included in this PR.

Also, at the moment you've commented out those lines, if the idea is to remove them, can you just fully delete them from the code.

@BrianCollinss
Copy link
Member Author

BrianCollinss commented Mar 21, 2024

These two tests do not exist in the Leaf class. I uploaded some simulations where these tests failed. So, either these tests should not be used (as per in the Leaf class) or something is wrong in calculating N reallocation in SimpleLeaf. I removed the troublesome tests and replaced them with a single test from the Leaf class.

@BrianCollinss
Copy link
Member Author

This one is either a very simple fix or a sign of a major issue. In either case, I will be happy to close it if not of interest.

@par456
Copy link
Collaborator

par456 commented Apr 2, 2024

We aren't sure of that ourselves. The person we need to have a look at this has been busy lately so we haven't been able to determine what action to take.

It would be good to leave this open a bit longer until they can check it, because if it is an error, we do want to have it fixed.

@BrianCollinss BrianCollinss changed the title Removed unnecessary N reallocation/retranslocation tests in SimpleLeaf Fix N reallocation/retranslocation issue in SimpleLeaf Apr 30, 2024
@BrianCollinss
Copy link
Member Author

A friendly reminder of this.

@BrianCollinss
Copy link
Member Author

Hey @par456 . This has been here for four months now. Any chance someone with a better grasp of internal processes can have a look? It has been reported in three crops so far.

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.

N retranslocation exceeds storage + metabolic nitrogen in organ: Leaf
3 participants