Skip to content

Conversation

@coxipi
Copy link
Contributor

@coxipi coxipi commented Apr 24, 2025

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 applicable) Tests have been added.
  • This PR does not seem to break the templates.
  • 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?

  • Omegaconf interpolations can be used in configs

Does this PR introduce a breaking change?

This is optional, but I changed the templates by assuming omegaconf would be instaled

Other information:

This doesn't replace all the functions in xscen.config. Rather, it's a small add-on that lets me use the interpolations of OmegaConf.

Currently, this is a bit dirty, because in config.yaml I make reference to keys from paths.yaml which is not explicitly referenced. Implicitly, we know we will merge config.yaml and paths.yaml, so it's fine. A proper way to do this would be to add it as a dependence:

# config.yaml
defaults:
  - paths.yaml

@coxipi coxipi changed the title Add support for omegaconf in configs Add support for omegaconf interpolations in configs Apr 24, 2025
@aulemahal
Copy link
Collaborator

Peut-être que ceci mérite une discussion plus large, mais je voterais pour une réécriture complète de config.py pour utiliser Omegaconf nativement partout, en remplacement de ce qu'on a déjà.

@coxipi
Copy link
Contributor Author

coxipi commented Apr 24, 2025

Peut-être que ceci mérite une discussion plus large, mais je voterais pour une réécriture complète de config.py pour utiliser Omegaconf nativement partout, en remplacement de ce qu'on a déjà.

Ok, oui il y a beaucoup de redondances en effet. Si on est à l'aise d'assumer cet dépendance, pourquoi pas

@aulemahal
Copy link
Collaborator

I make reference to keys from paths.yaml

Un autre moyen de s'en sortir serait d'utiliser les variables d'env ou une CLI pour lire les quelques variables "privées" qui restent ? Ça éliminerait le besoin d'un fichier paths.yml extra, mais demanderait un changement dans les habitudes d'appel de xscen.

@RondeauG
Copy link
Collaborator

Peut-être que ceci mérite une discussion plus large, mais je voterais pour une réécriture complète de config.py pour utiliser Omegaconf nativement partout, en remplacement de ce qu'on a déjà.

Ok, oui il y a beaucoup de redondances en effet. Si on est à l'aise d'assumer cet dépendance, pourquoi pas

Je n'ai pas d'opinion forte, je vous fait confiance là-dessus.

@coxipi
Copy link
Contributor Author

coxipi commented Apr 24, 2025

Si on veut aller all-in, hydra permet des choses comme:

# main.py 
@hydra.main(config_path="../configs/", config_name="config", version_base=None)
def train(cfg):
   # blabla
python main.py +paths.indicators=/path/to/your/indicators.yaml
# config.yaml, updatée avec l'appel python main.py

## ceci est ajouté avec l'appel de main.py
paths: 
  indicators: path/to/your/indicators.yaml

## ceci est déja codé
indicator:
  load_xclim_module:
    filename: ${paths.indicators}

Mais c'est pas mieux vraiment. config.yaml serait quand même écrit avec des références inexistentes. Je préfère l'approche d'avoir un paths.yaml

@coxipi
Copy link
Contributor Author

coxipi commented Apr 24, 2025

hydra permet aussi des instantiations. Tu dis quelles fonction ou classe vont avec tes arguments.

biasadjust:
  variables:
    snw: 
      training:
        _target_: xscen.train
        var: snw
        period: *ref_period
        method: EmpiricalQuantileMapping
        jitter_under:
          thresh: 1e-8 kg m-2
        ...

et tu instanties ça dans le script

from hydra.utils import instantiate
import xscen
task = "biasadjust"
for var in CONFIG[task]["variables"]:
   # instantiate(CONFIGtask]["variables"]["train"]) te retourne un callable qui manque dref & dhist
   ds_tr = instantiate(CONFIG[task]["variables"][var]["training"])(dref = dsref, dhist = ds)

mais c'est peut-être pas super utile comme abstraction

C'est pas tant différent de caller xs.train avec les arguments d'intérêt appelés avec le dict. Mais pour la config de pins qui avait des quantiles un peu funky, c'est quand même nice, l'information peut facilement être mise dans ma config aussi.

nquantiles: 
  _target_: numpy.arange
  start: 0.05
  stop: 1
  step: 0.02

Je vous laisse juger si c'est épatant ou dégoûtant héhé

@coxipi
Copy link
Contributor Author

coxipi commented Apr 24, 2025

Je commence à changer mon fusil d'épaule sur la malpropreté de mon approche actuelle, du moins relative. Je maintiens que le plus clean serait d'avoir un defaults qui fait référence à paths.yaml. Mais c'est pas vraiment pire que ce qu'on a en ce moment? Présentement, on définit properties_and_measures dans config.yaml, mais c'est sous-entendu que l'argument properties, un arg obligatoire, doit être ajouté via le paths.yaml. Donc, vraiment, je dirais que cette implémentation actuelle est pas plus dirty, et a le mérite d'être plus facile à comprendre (de mon point vue) et moins redondante

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants