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

Kd waterfuns #159

Merged
merged 109 commits into from Oct 19, 2023
Merged

Kd waterfuns #159

merged 109 commits into from Oct 19, 2023

Conversation

KeesDoolNMI
Copy link
Contributor

Functions for evaluating the effect of soil quality on water quality and quantity

@gerardhros gerardhros self-requested a review February 9, 2023 13:52
Copy link
Contributor

@gerardhros gerardhros left a comment

Choose a reason for hiding this comment

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

that was a substantial update, and merging this in multiple steps might have been easier.
All water funs are present now given the latest study of Kees (in collaboration with KWR).
These functions are out commented in wrapper functions obic, obic_field and obic_field_dt as well the associated test functions.
All other tests were succesfull. Ready to merge!

@SvenVw SvenVw changed the base branch from development to master October 18, 2023 11:15
R/groundwater_recharge.R Show resolved Hide resolved
R/groundwater_recharge.R Show resolved Hide resolved
R/nitrogen_efficiency.R Show resolved Hide resolved
R/nitrogen_efficiency.R Show resolved Hide resolved
R/nitrogen_efficiency.R Outdated Show resolved Hide resolved
R/precipitation_surplus.R Outdated Show resolved Hide resolved
vignettes/water_funs_references.bib Outdated Show resolved Hide resolved
@gerardhros
Copy link
Contributor

@KeesDoolNMI , can you answer the issues raised by Sven, and update references where needed?

NEWS.md Outdated
* N use norms of 1926 and 1927 (agrarisch natuurmengsel and overige akkerbouwgewassen), increased to match "Akkerbouwgewassen, overig" (RVO, 2022)

## Deprecated
* Building and modifying crops.obic from a script in OBIC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me it seems that this already happened in an earlier update of OBIC. The old script came along when merging the master into this branch. So this does not have to be added in the changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same for first point under Added, these crops are already available in main

Copy link
Member

@SvenVw SvenVw left a comment

Choose a reason for hiding this comment

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

Looks good, just make sure to fix failing unit tests

evaluate_logistic(D_PSP,0.04,180,2),
evaluate_logistic(D_PSP,0.05,300,2.5))]
# Calculate indicator score
dt[,I_PSP := evaluate_logistic(D_PSP,0.05,300,2.5)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked how did this in the pilot and I saw that we used the same score for grassland and cropland. So I removed the separate evaluation of grassland and cropland

@KeesDoolNMI KeesDoolNMI merged commit 831ced0 into master Oct 19, 2023
7 checks passed
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.

None yet

5 participants