Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modélisation hydrologique par classes ou par dictionnaire #93

Closed
1 task done
RondeauG opened this issue Feb 15, 2024 · 3 comments · Fixed by #18
Closed
1 task done

Modélisation hydrologique par classes ou par dictionnaire #93

RondeauG opened this issue Feb 15, 2024 · 3 comments · Fixed by #18
Labels
enhancement New feature or request

Comments

@RondeauG
Copy link
Collaborator

RondeauG commented Feb 15, 2024

Addressing a Problem?

Actuellement, la modélisation hydrologique est gérée par une fonction qsim = run_hydrological_model() qui utilise un dictionnaire model_config en entrée et retourne des débits. Cela fonctionne correctement pour un modèle simple à la GR4J, mais devient très compliqué très vite pour un modèle plus complexe à la Hydrotel ou Raven, où une bonne partie des paramètres et des fonctionnalités se cachent dans des fichiers de configuration.

La localisation et le nom des fichiers pertinents (météo, sorties) dépend d'informations qui se trouvent à travers quelques fichiers CSV.

Potential Solution

Peu importe notre décision, dans le cas d'Hydrotel, model_config devra contenir des paramètres comme simulation_options ou output_options pour permettre de consulter et modifier les fichiers CSV.

Solution 1: Continuer avec 'approche par dictionnaire
La liste de fonctions actuelle n'est pas suffisante. On aura absolument besoin de coder des fonctions supplémentaires.

  1. qsim = run_hydrological_model(model_config, return_outputs=True) pour exécuter le modèle et retourner des débits.
  2. ds_in = get_inputs(model_config) pour retrouver les bons fichiers d'entrée.
  3. qsim = get_streamflow(model_config) pour retrouver le bon fichier et retourner des débits, après que le modèle ait été exécuté.

Bref, model_config est toujours un intrant nécessaire. Un enjeu est qu'il pourrait être difficile d'avoir une fonction unique, puisque certains modèles pourraient demander des arguments supplémentaire.

Solution 2: Implémenter une classe avec une liste prédéfinie de fonctions
Je ne vois pas d'enjeu à garder l'approche model_config ici. Une fois le modèle initialisé, on pourrait avoir une liste de sous-fonctions similaires à la Solution 1:

  1. model = HydrologicalModel(model="Hydrotel", model_config)
  2. qsim = model.run(return_outputs=True) pour exécuter le modèle et retourner des débits.
  3. ds_in = model.get_inputs() pour retrouver les bons fichiers d'entrée.
  4. qsim = model.get_streamflow() pour retrouver le bon fichier et retourner des débits, après que le modèle ait été exécuté.

Ici, model_config est seulement utilisé une fois, car ses attributs sont ajoutés à la classe pendant un __init__(). Ça simplifie potentiellement les appels aux autres fonctions. Cela ouvre aussi la porte à plus facilement avoir une liste de paramètres qui diffère d'un modèle hydrologique à un autre pour les fonctions comme .get_inputs(), mais on voudra probablement éviter cela au maximum pour ne pas ajouter trop de complexité...

Contribution

  • I would be willing/able to open a Pull Request to contribute this feature.
@RondeauG RondeauG added the enhancement New feature or request label Feb 15, 2024
@RondeauG RondeauG changed the title Modélisation hydrologique par classes plutôt que par dictionnaire Modélisation hydrologique par classes ou par dictionnaire Feb 15, 2024
@richardarsenault
Copy link
Contributor

Merci @RondeauG pour l'analyse!

Je crois que ça peut être une bonne option d'y aller avec la solution 2. Cependant, pour garder ça le plus clair possible, je pense qu'il est important de garder les étapes de modélisation séparées le plus possible (i.e. comme tu le fais, c-a-d séparer get-inputs, run_model, get_outputs, etc.). Ça va permettre de garder les choses plus claires pour les usagers moins avancés que des méthodes complexes et des superclasses etc. que j'ai eu à gérer dans le passé et qui étaient vraiment obscures.

Bref, si c'est plus simple pour Hydrotel et que ça s'ajuste bien aux autres modèles, je crois que c'est la solution à privilégier.

@RondeauG
Copy link
Collaborator Author

RondeauG commented Feb 20, 2024

Voici une version très simplifiée de ce que le code pourrais avoir l'air dans les deux options:

Approche par fonctions/dictionnaire:

from ._hydrotel import Hydrotel

def run_hydrological_model(model_config: dict):
    if model_config["model_name"] == "Dummy":
        qsim = _dummy_model(model_config)
    elif model_config["model_name"] == "Hydrotel":
        qsim = Hydrotel(**model_config)
    else:
        raise NotImplementedError(
            f"The model '{model_config['model_name']}' is not recognized."
        )

    return qsim

def get_input(model_config: dict):
    if model_config["model_name"] == "Dummy":
        ds = _dummy_model_input(model_config)
    elif model_config["model_name"] == "Hydrotel":
        ds = Hydrotel(**model_config).get_input()
    else:
        raise NotImplementedError(
            f"The model '{model_config['model_name']}' is not recognized."
        )

    return ds

(...)

Approche par classes:

from ._hydrotel import Hydrotel

class HydrologicalModel:
    """Hydrological model class.

    """
    def __init__(self, model_name: str, model_config: dict):
        self.model_name = model_name

        if model_name == "Hydrotel":
            self.model = Hydrotel(**model_config)
        elif model_name == "Dummy":
            self.model = _dummy_model(**model_config)
        else:
            raise NotImplementedError(f"The model '{model_name}' is not recognized.")
        
    def run(self, **kwargs):
        return self.model.run(**kwargs)
    
    def get_input(self, **kwargs):
        return self.model.get_input(**kwargs)

    def get_streamflow(self, **kwargs):
        return self.model.get_streamflow(**kwargs)

Bref dans l'approche par fonctions, la boucle if/else doit être appelée à chaque fois. Dans l'approche par classes, en autant qu'on s'assure que les modèles ont tous les mêmes fonctions, on a seulement besoin une fois de la boucle.

Un autre avantage potentiel de l'approche par classes est que si on veut absolument ajouter une fonction à seulement un modèle, un utilisateur pourrait toujours y accéder en faisant m.model.nouvelle_fonction(), même si elle n'est pas dans les fonctions publiques de HydrologicalModel.

@sebastienlanglois
Copy link
Contributor

Personnellement, pour la facilité d'utilisation pour l'utilisateur et pour les mêmes raisons énoncées par @RondeauG , je vote en faveur d'une approche par classe.

RondeauG added a commit that referenced this issue Apr 16, 2024
<!-- Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [x] This PR addresses an already opened issue (for bug fixes /
features)
  - This PR fixes #93. 
- [x] (If applicable) Documentation has been added / updated (for bug
fixes / features).
- [x] (If applicable) Tests have been added.
- [x] HISTORY.rst has been updated (with summary of main changes).
- [x] Link to issue (:issue:`number`) and pull request (:pull:`number`)
has been added.

### What kind of change does this PR introduce?

* Changed the hydrological modelling module to use classes instead of a
dictionary,
* Code to prepare, then run a Hydrotel project.
* Partial standardisation of the outputs. The 'Atlas-style' formatting
is performed elsewhere.
* Added a new notebook to describe hydrological modelling.

### Does this PR introduce a breaking change?

- Kinda. `hydrological_modelling` has changed substantially, but was
very new.

### Other information:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants