-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add phosphorus to litter model #539
Conversation
…l chemistry changes
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #539 +/- ##
===========================================
- Coverage 94.98% 94.95% -0.03%
===========================================
Files 74 74
Lines 4145 4186 +41
===========================================
+ Hits 3937 3975 +38
- Misses 208 211 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. IT follows a similar pattern than nitrogen and, to be honest, I've not spotted that many repetitions to be worrisome.
@@ -106,6 +106,21 @@ var_name = "c_n_ratio_below_metabolic" | |||
[[core.data.variable]] | |||
file = "../data/example_litter_data.nc" | |||
var_name = "c_n_ratio_below_structural" | |||
[[core.data.variable]] | |||
file = "../data/example_litter_data.nc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a generic question and nothing to do with this PR in particular. For what I can see, running a full Virtual Ecosystem simulation requires A LOT of data to be available. Is that data typically available or easy to find? Are you planning to provide defaults for them - some, at least? If the data can be downloaded from online repositories (for free), would you be including hooks to download that data in the background?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a good question. In general, a lot of this data will not be particularly easy to get, partly because ecological data tends to get reported in fairly inconsistent ways. But also because (at least in the soil and litter models) some variables are chosen because they make sense in the model context rather than being commonly measured empirically, e.g. structural litter isn't measured empirically, mass of dead leaves, twigs, branches etc is.
A few more (Malaysia based) postdocs have joined the team and their role will be getting the data needed for parameterisation, calibration and validation together. I'm not sure of the exact planned workflow there, but I believe the idea is to have all the scripts used to wrangle the data and the final datasets ready for input available online. So this probably make sense to be hooked in some way to the model itself, particularly for the non-site specific data.
Description
This pull requests adds tracking of litter phosphorus concentrations. This follows an identical structure to the nitrogen case. I did consider trying to make a generic functions rather than duplicating functions, but it didn't feel worth the effort as we have no current plans to add further nutrients (e.g. Potassium).
As there weren't any structural changes I didn't split this up into separate PRs, hopefully it is a manageable size as a single PR.
Fixes #512
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks