Pmp #176
Pmp #176
Conversation
…gest_values are included.
…ey reproduce the results. The spatial_storm_aggregation is not ready yet.
…average_storm_configurations
…re the merge request. The bottleneck is in the xr.concat
|
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
Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨ |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
You might want to investigate the numpy testing utilities. More powerful tests (faster too, I would imagine): https://numpy.org/doc/stable/reference/routines.testing.html
There was a problem hiding this comment.
Indeed. The current test are also never checking the results themselves to make sure that they are OK or that they don't change when we update the code.
https://github.com/hydrologie/xhydro/blob/main/tests/test_local.py for inspiration.
There was a problem hiding this comment.
I included numpy utilities in the tests. They also check the results numerically to make sure they do not change if the code is updated.
| @@ -169,6 +169,7 @@ jobs: | |||
| create-args: >- | |||
| mamba | |||
| python=${{ matrix.python-version }} | |||
| lmoments3 | |||
There was a problem hiding this comment.
Perhaps we can have a specific build that installs lmoments3. I can look into implementing that.
There was a problem hiding this comment.
Good job ! Par contre, j'ai un questionnement de fond sur l'endroit où ce code devrait se trouver.
- 1: On garde ça comme ça.
- 2: On garde ça dans
xhydro, mais on déplace versindicators.py. Il y a déjà des indicateurs similaires (compute_volume,get_yearly_op), donc je crois que ça peut fitter là plutôt que d'avoir son propre fichier. - 3: On bouge ça carrément dans
xclim. Selon moi ça a clairement sa place là si on veut, mais ça va demander un peu plus de travail. On pourrait avoir un petit wrapper dansxhydroau besoin.
Thoughts? @ospinajulian @elysefournier
There was a problem hiding this comment.
Indeed. The current test are also never checking the results themselves to make sure that they are OK or that they don't change when we update the code.
https://github.com/hydrologie/xhydro/blob/main/tests/test_local.py for inspiration.
src/xhydro/pmp.py
Outdated
| return da_higest | ||
|
|
||
|
|
||
| def precipitable_water(ds, ds_fx, acc_day=[1], path=None): |
There was a problem hiding this comment.
Je vois plusieurs améliorations possibles à tout ce qui suit dans ce fichier, pour suivre une mécanique plus près de xclim, mais ça serait long et pas convivial à mettre dans GitHub... Est-ce correct si je modifie directement la branche @ospinajulian ?
There was a problem hiding this comment.
Oui, aucun problème @RondeauG
There was a problem hiding this comment.
La dernière fois que @RondeauG a proposé de faire quelques modifications à une PR, ça a donné un super résultat ;)
Co-authored-by: Trevor James Smith <[email protected]>
Co-authored-by: RondeauG <[email protected]>
Personnellement, je pencherais pour 1 ou 2 (avec préférence pour l'option 2). L'objectif d'une PMP est quand même très "appliquée" et sert souvent seulement d'intrant pour l'étape suivante qui est la modélisation hydrologique et n'a pas vraiment d'utilité dans le monde de la climatologie par elle-même. Si tu penses pouvoir harmoniser le code avec les indicateurs lors de ton refactor, je pense que ce serait super. À très long terme, si on voit que ça a vraiment plus sa place dans |
Pull Request Checklist:
What kind of change does this PR introduce?
A new pmp module has been added to compute the Probable Maximum Precipitation
Does this PR introduce a breaking change?
No