Add potential evapotranspiration method based on Droogers and Allen (2002)

[JavierDiezSierra]

Pull Request Checklist:

• This PR addresses an already opened issue (for bug fixes / features)
  • This PR closes PET calculation with the Hargreaves Modificated method (Droogers and Allen, 2002) #1710
• Tests for the changes have been added (for bug fixes / features)
  • (If applicable) Documentation has been added / updated (for bug fixes / features)
• CHANGES.rst has been updated (with summary of main changes)
  • Link to issue (:issue:1710) and pull request (:pull:1723) has been added

What kind of change does this PR introduce?

This change introduces a new method to calculate potential evapotranspiration based on Droogers and Allen (2002). The method incorporates precipitation as a proxy for humidity and is more appropriate for dry regions

Does this PR introduce a breaking change?

No

Other information:

[github-actions]

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

• The relevant author information has been added to AUTHORS.rst and .zenodo.json

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

[aulemahal]

Thanks for the PR!

Please look at my commit to see if you agree. We try to avoid exact frequency matching in indices. Xclim's Indicators are the objects that may enforce strong rules about data frequency, but indices should let the user do what it wants.

So instead of deciding what to do according to the infer_freq, I used the same idea as in the TW48 implementation : resample to monthly no matter what. If the data is already monthly, this will be a no-op. I also generalized the function to get a daily time coord from monthly data, as used by TW48 also.

One caveat of my technique is the chunksizes that are not easily inferable. I took a guess that passing the same chunks as monthly tasmin would be ok in most cases. Especially because we resample the radiation right away.

Would you be able to add a test case for this new method ? The test should be a method of class TestPotentialEvapotranspiration, line 3159 of tests/test_indices.py.

[Zeitsperre]

@aulemahal I think that I have the workflows configured to trigger on any comment. This might be why the builds often cancel during reviews. Will open an issue.

[coveralls]

coverage: 90.782% (+0.01%) from 90.769%
when pulling 59f5d59 on JavierDiezSierra:da02
into 9a87705 on Ouranosinc:main.

[JavierDiezSierra]

Thanks for the PR!

Please look at my commit to see if you agree. We try to avoid exact frequency matching in indices. Xclim's Indicators are the objects that may enforce strong rules about data frequency, but indices should let the user do what it wants.

So instead of deciding what to do according to the infer_freq, I used the same idea as in the TW48 implementation : resample to monthly no matter what. If the data is already monthly, this will be a no-op. I also generalized the function to get a daily time coord from monthly data, as used by TW48 also.

One caveat of my technique is the chunksizes that are not easily inferable. I took a guess that passing the same chunks as monthly tasmin would be ok in most cases. Especially because we resample the radiation right away.

Would you be able to add a test case for this new method ? The test should be a method of class TestPotentialEvapotranspiration, line 3159 of tests/test_indices.py.

Thank you for the proposed changes. It seems correct to resample everything to monthly and work at that resolution. Regarding the issue of chunksize when calculating solar radiation, it was causing problems for me, so I have removed it. On the other hand, I have modified the term lv=2.5 and included the constant 0.408 to align the Hargreaves (HG85) method with the new modified method (DA02). The result of applying 1/2.5 = 0.4 is very similar to multiplying by 0.408, but this way we use the original constant that appears in Hargreaves et al (2002).

Finally, I have added the test for the new method, although when running all tests with pytest, I encountered many errors in many other tests, although perhaps it's my problem and I haven't followed the correct procedure.

[Zeitsperre]

@JavierDiezSierra This is looking good!

Once the failing tests are addressed, we should be able to move forward with the merge in advance of the next release (v0.49.0).

All the best!

[aulemahal]

@JavierDiezSierra I think I fixed the tests in the last commit. This looks good to go.

[JavierDiezSierra]

@JavierDiezSierra I think I fixed the tests in the last commit. This looks good to go.

¡Many thanks for you help!.

[Zeitsperre]

@JavierDiezSierra

Thanks so much for your pull request!