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

OBI-16 #184

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

OBI-16 #184

wants to merge 2 commits into from

Conversation

SvenVw
Copy link
Member

@SvenVw SvenVw commented Jun 2, 2023

Fixed

  • Fix calculation of slv by removing the not applicable conversion from g S /kg to mg S /kg for A_S_RT [OBI-16]

@SvenVw SvenVw self-assigned this Jun 6, 2023
@SvenVw SvenVw requested a review from gerardhros June 6, 2023 11:11
@@ -71,11 +71,11 @@ calc_slv <- function(B_LU_BRP, B_SOILTYPE_AGR, B_AER_CBS,A_SOM_LOI,A_S_RT, D_BDS

# Calculate SLV for grass (deze formule: Stot = g/kg en dichtheid in g/cm3)
dt.grass <- dt[crop_category == "grasland"]
dt.grass[, value := 17.8 * A_S_RT * 0.001 * D_BDS * 0.001]
dt.grass[, value := 17.8 * A_S_RT * D_BDS * 0.001]

Copy link
Contributor

@gerardhros gerardhros Jun 6, 2023

Choose a reason for hiding this comment

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

This is incorrect, see page 18 in Bemestingsadvies grasland. Soils are low in S supply when S supplying capacity is lower than 17 and its high when SLV exceeds the 23 (for grassland). If S total is 650 mg/kg (a realistic value), then SLV is around 15 kg / ha. If you remove this correction, then this change would result in a S supply of 15.000 kg S/ha, that seems unrealistic to me. Or is the unit of D_BDS different here?


# Calculate SLV for maize for 0-25 cm depth
dt.maize <- dt[crop_category == "mais"]
dt.maize[, value := 41.2 * A_S_RT * 0.001 * D_BDS * 2.5 * 0.001]
dt.maize[, value := 41.2 * A_S_RT * D_BDS * 2.5 * 0.001]
# correction for the length of growing season (43.5%)
Copy link
Contributor

Choose a reason for hiding this comment

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

here the same. if S is too low, there shoulde be another reason than this unit correction

@@ -90,7 +90,7 @@ calc_slv <- function(B_LU_BRP, B_SOILTYPE_AGR, B_AER_CBS,A_SOM_LOI,A_S_RT, D_BDS

# calculate C-stock (kg/ ha) and CS ratio (sven: check unit)
dt.arable[,D_OC := A_SOM_LOI * 100 * 100 * 0.3 * D_BDS * 0.01]
dt.arable[,A_CS_RAT := A_SOM_LOI * 10 / (A_S_RT * 0.001)]
dt.arable[,A_CS_RAT := A_SOM_LOI * 10 / (A_S_RT)]

Copy link
Contributor

Choose a reason for hiding this comment

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

no this is incorrect. CS_RAT is the SOM-to-S ratio. Note that this is rather SOM stock than SOC, but anyhow, A_SOM_LOI times 10 results in units g/kg, so when dividing by A_S_RT * 0.001 the resulting unit is dimensionless.

Copy link
Contributor

Choose a reason for hiding this comment

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

note that sulpher is depreciated and sulfer is the new one. So, you updated the old file.

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.

i don't think that the low SLV is an issue due to the units here.
the equations in original are according to the current guidelines in fertilizer recommendation.
removing the 0.001 as done in this repo results in SLV values exceeding 1000 kg SLV / ha, very unrealistic. The values should vary around 5 to 50 kg S / ha for grassland and maize at least, and they do.

@SvenVw
Copy link
Member Author

SvenVw commented Jun 7, 2023

Ok, thanks for checking. Is then maybe because the evaluation function is incorrect:

dt.arable[,value := evaluate_logistic(sbal, b = 0.5, x0 = -4, v = 3)]

Grass and maize use the same parameters, so can we not use those parameters for arable as well?

dt.maize[,value := evaluate_logistic(D_SLV, b = 0.29, x0 = 15, v = 1.7)]

dt.grass[,value := evaluate_logistic(D_SLV, b = 0.29, x0 = 15, v = 1.7)]

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

2 participants