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

Workability depth #193

Open
wants to merge 11 commits into
base: development
Choose a base branch
from
Open

Workability depth #193

wants to merge 11 commits into from

Conversation

Sies99
Copy link

@Sies99 Sies99 commented Dec 11, 2023

Fixing capilary rise distance for determining workability by adding spring_depth to ZCrit.

@Sies99 Sies99 requested a review from BrentWHR December 11, 2023 10:02
@Sies99 Sies99 self-assigned this Dec 11, 2023
R/workability.R Outdated Show resolved Hide resolved
R/workability.R Outdated
#' @param B_SOILTYPE_AGR (character) The agricultural type of soil
#' @param B_GWL_GLG (numeric) The lowest groundwater level averaged over the most dry periods in 8 years in cm below ground level
#' @param B_GWL_GHG (numeric) The highest groundwater level averaged over the most wet periods in 8 years in cm below ground level
#' @param B_GWL_ZCRIT (numeric) The distance between ground level and groundwater level at which the groundwater can supply the soil surface with 2mm water per day (in cm)
Copy link
Contributor

Choose a reason for hiding this comment

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

B_GWL_ZCRIT should be defined as
"Distance between groundwater table and root zone (30 cm -mv) for delivering 2 mm/day"
As is the description of the input data we use.

This has consequences for the changes in line 117. For example, if the spring_workindepth == 30, the Zcrit (to the root zone 0-30cm) already accounts for this working depth and no addition or substraction is needed.

Copy link
Author

Choose a reason for hiding this comment

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

I pushed another script now that resolves this by firstly chaing the definition in line 15 and then changing the calculation we discussed to Zcrit + springdepth - 30. This way if the working depth is less than the rootzone the depth at which the capilary rise is lower than the evaporation is slightly decreased, if the working depth is larger than the rootzone this depth increases. Good thing is that with this definition the change in depth is smaller making the assumption the soil profile remains the same more reasonable.

I'm not sure if committing went well, please let me know. I can't see how I can ask you to review.

@BrentWHR
Copy link
Contributor

Feel free to add yourself as package contributor (ctb) in the DESCRIPTION file.

@Sies99 Sies99 requested a review from BrentWHR December 13, 2023 08:49
Copy link
Contributor

@BrentWHR BrentWHR 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 to me

@BrentWHR BrentWHR requested a review from SvenVw December 13, 2023 12:15
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.

@BrentWHR Is it possible to improve the git diff for R/workability.R? It is now hard to see which changes are made

@BrentWHR
Copy link
Contributor

@BrentWHR Is it possible to improve the git diff for R/workability.R? It is now hard to see which changes are made

That's weird, only a few lines should have changes. Looking at diffs of individual commits using GitKraken 🐙 it seems 46a01a2 added a trailing space to each line but I do not see all these trailing spaces myself when opening the script in Rstudio with the any of the commits.

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

3 participants