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

Master fix ningorg refill #283

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Master fix ningorg refill #283

merged 3 commits into from
Feb 4, 2025

Conversation

marcadella
Copy link
Collaborator

Undo changes made to Recover_N_balance by mistake when trying to fix the N leak. I misunderstood how the pools worked and implemented something silly which this PR is undoing.

In addition, the hardcoded ninorg content is set to init_Nmineral rather than the hardcoded value 0.2. The rest of the changes are just a revert to the original code.

Note that there is still an issue with N leaking leading to negative soil N pools, but fixing these should be treated as a separate issue (#256 ) after this PR has been merged to master to restore the original behavior.

marcadella added 3 commits February 3, 2025 11:37
…coded ninorg content is set to init_Nmineral rather than the hardcoded value 0.2. The rest of the changes are just a revert to the original code.
@marcadella marcadella self-assigned this Feb 3, 2025
@marcadella marcadella requested a review from fabern February 3, 2025 14:57
@fabern
Copy link
Member

fabern commented Feb 3, 2025

Which commit was the initial change? When was this bug introduced?

Found it, it was here: db7b5ca
That means it was introduced into v5.0 and v5.0.0.

  1. Can you increase the following in this PR?
  • the version number
  • the NEWS.md

Doing it all in this PR keeps this single commit PR atomic (i.e. consistent within itself).

  1. What is about the commit, that introduced vegn%ninorg%n14 = 0.2? You are now replacing this with init_soil%init_Nmineral, however before there was nothing. What is correct?

@marcadella
Copy link
Collaborator Author

db7b5ca

@marcadella marcadella merged commit 7be32ed into master Feb 4, 2025
7 checks passed
@marcadella marcadella deleted the master-fix-Ningorg-refill branch February 4, 2025 08:34
@fabern
Copy link
Member

fabern commented Feb 4, 2025

Hmm... I didn't understand why you have merged this without changing the NEWS.md? If this is a bugfix than the user on v5.0.0 should understand that there it is wrong and an update to a future 5.0.1 would fix this.

Moreover, what's the relation to the other PR #284?
Is this here a fix and the other a feature?

@marcadella
Copy link
Collaborator Author

marcadella commented Feb 4, 2025 via email

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