search

aquacropos / aquacrop

AquaCrop-OSPy: Python implementation of AquaCrop-OS
https://aquacropos.github.io/aquacrop/
Apache License 2.0
102 stars 73 forks source link

Fixing bugs with groundwater, irrigation, soil bunds, canopy and root development #162

Closed oleksmialyk closed 3 months ago

oleksmialyk commented 5 months ago

I was comparing AquaCrop-OSPy based on AquaCrop v6.1 (2022) vs v7.0 (2024) and found several bugs including:

  1. check_groundwater_table.py doesn't return NewCond_zGW variable leading to constant groundwater levels (see #99)
  2. HIref_current_day.py doesn't check for NewCond_CC_prev <= (Crop.CCmin*Crop.CCx) condition as it used to do in AquaCrop-OSPy v6.1 (perhaps it's how new AquaCrop v7 works, please confirm)
  3. irrigation.py has hardcoded 15mm irrigation for IrrMngt_IrrMethod == 1 (see #132)
  4. root_development.py doesn't return NewCond_rCor which is supposed to be updated daily according to AquaCrop-OSPy v6.1
  5. infiltration.py has soil bunds multiplied by 1000, although they are provided in mm already
oleksmialyk commented 3 months ago

The changes to run_single_timestep make sense and will be approved.

The changes to infiltration are incorrect, because the zBund is supplied in metres so should be converted to millimetres (1000) during these calculations. Please either remove these changes or provide evidence to support the removal of the '1000' before resubmitting this commit.

Thanks :) zBund in m would be fine if its use was consistent through the code. In some cases, it's compared to other variables in mm which leads to a mismatch in units. E.g:

Since the whole water balance is done in mm, we can make it consistent with zBund too so the users are less confused. Also, it's a bit strange that zBund is in m but bund_water (initial storage) in mm. Alternatively, you can search through all instances of zBund and z_bund to make sure it's everywhere multiplied by 1000. But again, this adds unnecessary operations to the code.

First of all, thank you for spotting a number of discrepancies in the code! Here are our comments:

In HIref_current_day, we purposefully made and tested the removal of L106-108 in this commit as part of the update from v6 to v7. Please remove this change before resubmitting.

The return of zGW in check_groundwater_table is correct and will be accepted.

The removal of hard-coded value in irrigation is correct and will be accepted.

The return of rCor in root_development is correct and will be accepted.

In run_single_timestep, all changes are correct apart from L273 which you have already fixed in the subsequent bunds-related commit.

I removed lines in HIref_current_day, thanks.