Getflowinst

[vinfort]

Pull Request Checklist:

• This PR addresses an already opened issue (for bug fixes / features)
  • This PR fixes #xyz
• (If applicable) Documentation has been added / updated (for bug fixes / features).
  • If changes affect the GIS, Raven Modelling, Extremes.jl or Use Case Example notebooks, they have been re-run (ReadTheDocs will not update these).
  • If text has changed, make initialize-translations / make-translations.bat has been run and translations have been updated.
• (If applicable) Tests have been added.
• CHANGELOG.rst has been updated (with summary of main changes).
  • Link to issue (:issue:number) and pull request (:pull:number) has been added.

What kind of change does this PR introduce?

Adds new feature in order to be able to read instantaneous flow data from ECCC and CEHQ stations

Does this PR introduce a breaking change?

No

Other information:

This is my first pull request. I created an issue and updated the change log but I may have missed a few thinkgs.

[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 ✨

[RondeauG]

Allô, merci pour la contribution ! Je doute d'avoir le temps de regarder la PR d'ici les vacances, mais je vais regarder ça en janvier.

[RondeauG]

J'ai eu l'occasion de regarder la PR.

D'un point de vue plus macro, la fonctionnalité d'aller chercher des données externes est un peu out-of-scope, mais on pourrait quand même l'ajouter. Je propose les options suivantes:

1. On dépose cela plutôt dans xdatasets (sous la même organisation que xHydro), qui correspond très bien à ce genre de tâche.
2. On garde cela ici, sous un nouveau sous-module data.
3. On dépose cela plutôt dans miranda, qui est l'outil que l'on a développé à Ouranos pour aller chercher des données climatiques. Par contre, c'est une librairie à vocation un peu plus interne que xdatasets ou xHydro.

Sinon, quelques commentaires généraux:

• Il faudrait retirer le texte de la licence GPL. Les librairies ont déjà leur propre fichier de licence. MIT pour xdatasets, Apache 2.0 pour xHydro.
• Les print devraient plutôt être des logger.info ou logger.debug.
• Il faudrait ajouter des tests.
• Il faudrait idéalement ajouter un exemple d'utilisation dans la documentation.

[vinfort]

Merci Gabriel, je suis ouvert à toutes ces suggestions! Je vous laisse décider ce que vous voulez faire avec ce code avant d'y retoucher.

[RondeauG]

Excellent. Sébastien est d'accord pour que l'on mette ça dans xdatasets et je trouve que c'est l'endroit le plus logique. Je pourrais continuer de faire le review là-bas.

[RondeauG]

xdatasets repose sur une classe Query (https://github.com/hydrologie/xdatasets/blob/main/src/xdatasets/core.py) qui va chercher des données en ligne qui sont déjà entreposées sur un serveur et prêtes pour le traitement, ce qui permet des appels du genre:

ds = xdatasets.Query(
        datasets= {
                "deh": {
                    "id": ["020*"],
                    "regulated": ["Natural"],
                    "variables": ["streamflow"],
                }
         },
         time= {"start": "1970-01-01", "minimum_duration": (15 * 365, "d")}
)

La bonne manière de faire ça demanderait un peu de travail. Ce que je verrais, ça serait d'avoir deux nouveaux "datasets" qui utiliseraient un nouveau backend fait de tes classes.
P.S. Les données du CEHQ/DEH sont déjà là, mais ne sont pas toujours complètement à jour. C'est donc utile d'avoir l'option d'aller les télécharger.

Fais-moi savoir si tu veux de l'accompagnement dans tout ça.

[vinfort]

C'est clair que je vais avoir besoin d'aide! Dispo jeudi PM pour en discuter (je peux passer la journée à Ouranos si tu es sur place).

[Zeitsperre]

This is a license that is incompatible with the one we currently have. I'm personally not opposed to changing the license if everyone else is onboard, but there are consequences for it. We would need to reexamine our stack to see how we can bundle this together in things like PAVICS.