-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding organic phosphorus flows #653
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #653 +/- ##
===========================================
- Coverage 94.63% 94.57% -0.06%
===========================================
Files 73 73
Lines 4619 4645 +26
===========================================
+ Hits 4371 4393 +22
- Misses 248 252 +4 ☔ 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.
The code itself looks OK - it pretty much follows what nitrogen is doing, as far as I can tell.
However, I've seen that now there are several functions returning dictionaries rather than tuples that can be unpacked, which is less readable. I wonder if it would be better to create dataclasses, as it is done for MicrobialChanges
, to contain those outputs.
The same might apply to pools
, which is a very large dict and that could be more more readable if it were a dataclass.
None of this is a request for a change, just observations.
@dalonsoa yes I did wonder whether returning dataclasses rather than dictionaries would be better practice. Good to know! The next thing I'm working on will be adding inorganic nitrogen and phosphorus pools in, which will involve altering some of functions added/changed here. With that in mind I will merge this now and switch outputs to dataclasses when I'm working on the new code |
Description
This pull request adds organic flows of phosphorus to the soil model. This involved extending the functionality developed to track the organic nitrogen cycle to also track the organic phosphorus cycle. The scientific content is a bit dense here, but would be good to get feedback on the code structure/clarity.
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks