Deltares / hydromt

HydroMT: Automated and reproducible model building and analysis
https://deltares.github.io/hydromt/
MIT License
68 stars 29 forks source link

pyet gives error during pet calculation penman-monteith #907

Closed shartgring closed 2 months ago

shartgring commented 5 months ago

HydroMT version checks

Reproducible Example

Using simple Penman-Monteith method to determine pet results in an error. pyet is used to calculate the actual vapor pressure. The wrong arguments are parsed to the pyet function or the wrong function is used. From the hydromt log:

2024-04-24 20:45:43,668 - update - forcing - INFO - Calculating Penman-Monteith ref evaporation
2024-04-24 20:45:43,683 - update - main - ERROR - calc_e0() got an unexpected keyword argument 'tmax'
Traceback (most recent call last):
  File "C:\Users\hartgrin\miniconda\envs\hydromt-wflow\Lib\site-packages\hydromt\cli\main.py", line 332, in update
    mod.update(model_out=model_out, opt=opt, forceful_overwrite=fo)
  File "C:\Users\hartgrin\miniconda\envs\hydromt-wflow\Lib\site-packages\hydromt\models\model_api.py", line 329, in update
    self._run_log_method(method, **kwargs)
  File "C:\Users\hartgrin\miniconda\envs\hydromt-wflow\Lib\site-packages\hydromt\models\model_api.py", line 188, in _run_log_method
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\hartgrin\miniconda\envs\hydromt-wflow\Lib\site-packages\hydromt_wflow\wflow.py", line 2349, in setup_temp_pet_forcing
    pet_out = hydromt.workflows.forcing.pet(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\hartgrin\miniconda\envs\hydromt-wflow\Lib\site-packages\hydromt\workflows\forcing.py", line 428, in pet
    pet_out = pm_fao56(
              ^^^^^^^^^
  File "C:\Users\hartgrin\miniconda\envs\hydromt-wflow\Lib\site-packages\hydromt\workflows\forcing.py", line 669, in pm_fao56
    avp = pyet.calc_e0(tmax=temp_max, tmin=temp_min, rh=temp_dew)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: calc_e0() got an unexpected keyword argument 'tmax'

And indeed, the following lines can be found in forcing.py:

    if var == "temp_dew":
        avp = pyet.calc_e0(tmean=temp_dew)
    elif var == "rh":
        avp = pyet.calc_e0(tmax=temp_max, tmin=temp_min, rh=temp_dew)

According to pyet documentation, pyet.calc_e0 takes only t_mean as argument. But maybe it should be pyet.calc_ea which calculated the actual vapor pressure? That one also accepts multitple arguments, including tmax, tmin, rh. See also https://github.com/pyet-org/pyet/blob/ff411787fbaeaa903b9966e28df41db9afb05a28/pyet/meteo_utils.py#L222

Note: I tried this using pyet version 1.3.1, hydromt version 0.9.4 and hydront_wflow version 0.5.0

Current behaviour

Gives an error when computing pet when not providing pressure term. The error is raised when pressure is calculated by pyet package

Desired behaviour

Change arguments for pyet.calc_e0 or use pyet.calc_ea

Additional context

No response

savente93 commented 5 months ago

Hey @shartgring thank you for reporting this issue. The core team is really busy at the moment preparing for v1, so I'm not sure when we'll have time to address this issue, but given that you seem to have done some diagnosing yourself, would you be interesting in making a PR to fix this yourself? If you're not sure how to go about the PR I'm happy to walk you through it.

shartgring commented 5 months ago

Sounds good Sam! I have made a fork as I needed the implementation this week. I will work a bit more on it and then see if I can share that. I'll come back to you if I encounter any problems or when I need some help :)

shartgring commented 5 months ago

@savente93 I fixed it on my own fork and tested it within my own project. Seems to work like a charm. I created a pull request https://github.com/Deltares/hydromt/pull/918 I hope the PR looks okay and otherwise let me know how I can improve it :)

shartgring commented 5 months ago

The PET workflow using pyet can still be improved for readability as variables get mixed up. However in terms of fixing that the error is not raised anymore, the fix in the PR is sufficient

savente93 commented 5 months ago

I'll try and have a look at it today. thanks!