-
Notifications
You must be signed in to change notification settings - Fork 168
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
base: master
Are you sure you want to change the base?
Fix N reallocation/retranslocation issue in SimpleLeaf #8726
Conversation
@hut104 Can you have a look at this one? |
Hey @hut104. Eagerly looking forward to your comments! :) |
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. |
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. |
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. |
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. |
A friendly reminder of this. |
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. |
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.