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

Remove ESM15 special case related to lnonwood condition on dNplantdt(wood) #434

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rml599gh
Copy link
Contributor

@rml599gh rml599gh commented Oct 25, 2024

CABLE

Thank you for submitting a pull request to the CABLE Project.

Description

Non-ESM15 code versions had a condition on the calculation of dNplantdt(:.wood) to only do the calculation for woody pfts, having first set it to zero for all pfts. An ifndef was used to ignore the condition when ESM15 was defined YES.
The was no apparent reason why ESM15 could not use the condition so the special case was removed.
For clarity the condition was re-written as an IF/THEN.
dPplantdt(:,wood) is calculated in the same way as dNplantdt but had no condition. For consistency, a similar condition has been added so that nitrogen and phosphorus are treated the same way.

Fixes #423

Type of change

Please delete options that are not relevant.

  • [X ] Bug fix / code clean up
  • New or updated documentation

Checklist

  • The new content is accessible and located in the appropriate section.
  • I have checked that links are valid and point to the intended content.
  • [X ] I have checked my code/text and corrected any misspellings

Please add a reviewer when ready for review.


📚 Documentation preview 📚: https://cable--434.org.readthedocs.build/en/434/

@rml599gh rml599gh linked an issue Oct 25, 2024 that may be closed by this pull request
@rml599gh
Copy link
Contributor Author

Testing consisted of 1 month ESM1.5 runs under piControl conditions.

Initial tests were difficult to interpret as the simulations drifted apart after 2-3 days. Turning off the LAI and vcmax feedbacks gave much clearer results as changes in the carbon cycle did not feedback to the climate.
Results from three tests are noted here.
423b: Code from main i.e. no change
423c: Removal of ESM15 case
423d: Change to IF/THEN for dNplantdt. Addition of condition for dPplantdt

This table shows global maximum (3 columns) and global average for Nitrogen wood pool at end of January for 423b, 423c, 423d.
image

As expected, woody pfts show no change across cases. Non-woody pfts are zero (not sure about bare ground but no change across tests).

However non-woody pfts are not exactly zero.
image
image
There are differences between 423b and 423c, showing the impact of including the condition. There is no difference between 423c and 423d since this is just a change in how the nitrogen code is written.

Similar results are seen for the Phosphorus wood pool. No difference for woody pfts between the tests.
For non-woody:
image
image
423b and 423c are the same. 423d is different due to the addition of the lnonwood condition for dPplantdt.

@rml599gh rml599gh requested review from a team and SeanBryan51 and removed request for a team October 25, 2024 04:51
Copy link
Collaborator

@SeanBryan51 SeanBryan51 left a comment

Choose a reason for hiding this comment

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

I can't really comment on the differences you are getting in the model output but from a technical view it looks good to me. Just on the test cases you mentioned:

423b: Code from main i.e. no change
423c: Removal of ESM15 case
423d: Change to IF/THEN for dNplantdt. Addition of condition for dPplantdt

It would be great if each of these changes (423c and 423d) are separate commits so the differences in code between each case is visible to the reviewer.

Comment on lines +1029 to +1030
!R. Law 25/10/24 removed ESM15 case as no need to exclude the condition
!that is in offline/trunk. Also re-write as IF / THEN
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a general comment: I would write this in the commit message rather than the actual source code as a commit message should explain why you made the change. I try to reserve code comments for explaining why the code does what it does to help future developers looking at the code (e.g. why we check for casamet%lnonwood(npt)==0 when setting casapool%dNplantdt(npt,wood)).

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.

ESM1.5 reconciliation: lnonwood condition
2 participants