Ouranosinc / xscen

A climate change scenario-building analysis framework.
https://xscen.readthedocs.io/
Apache License 2.0
15 stars 2 forks source link

More intuitive formatting for 'periods' #87

Closed RondeauG closed 1 year ago

RondeauG commented 2 years ago

Addressing a Problem?

periods is used across multiple functions in xscen, but it is not always easy to know which format is accepted or not. Sometimes [start, end] will work, sometimes it needs to be a list of lists [[start, end]]. Sometimes int works, sometimes str are required... It's honestly quite a mess that we should improve.

Potential Solution

Support multiple formats (list & list of lists, int & str) for all functions that have a periods argument.

Additional context

No response

Contribution

juliettelavoie commented 2 years ago

I think this is a good idea.

If some functions really can't have many periods, depending on the function, we could have: period: only 1 period accepted (only [start, end] accepeted) or periods: 1 or many ( [start, end]or [[start, end]] accepted)

Are ints accepted as years or as index isel ?

RondeauG commented 2 years ago

I'd have to double-check, but I think int instances are simply converted to strings, then used in .sel(time=slice(period[0], period[1])). It's just that this conversion is not always done (or bugged?) and will fail for some functions.

juliettelavoie commented 2 years ago

Mais pourquoi on fait le casting int to string? perso, j'irais pour accepter juste des strings comme xarray...

RondeauG commented 2 years ago

Ben, ça fait un peu partie du problème. slice utilise en effet juste des strings, mais nous on supporte parfois plus, parfois pas.

juliettelavoie commented 2 years ago

ok je vote pour ne jamais supporter, je trouve ça mélangeant pour rien.

aulemahal commented 2 years ago

Je sais pas si ça pourrait s'insérer ici, mais dans ma copie locale j'ai fais quelques modifs. J'avais besoin de changer le coverage dans extract._subset_file_coverage, mais l'argument est inacessible depuis search_data_catalogs. Faque j'ai ajouté une 3e élément à period. Si présent, c'est ce chiffre qui est utilisé comme coverage. Qu'en pensez-vous?

Les alternatives auxquelles j'ai pensé:

RondeauG commented 2 years ago

C'est quoi, le 3e élément de period? Sinon, je crois que l'option 1 serait celle qui est la plus instinctive.

juliettelavoie commented 2 years ago

Moi aussi je préfère ajouter coverage comme argument.

aulemahal commented 2 years ago

Ben correct pour moi. Ma technique avec periods=[1981, 2010, 0.95] permettait de ne rien modifier du tout dans search_data_catalogs (sauf la doc mettons). Mais c'est bien moins explicite, j'en conviens absolument.

Pour l'info, l'erreur venait de fichier du MRCC5 qui sont par mois. Notre utilisation de "Period(..., freq='H')" implique qu'un date_end de données quotidiennes (ou plus coarse) est à 23H. Sur ma période, pour chaque mois, il manquait 1h et ça a donné 96% de coverage.