Calendar backlog items - Githubissues

AI4S2S / s2spy

A high-level python package integrating expert knowledge and artificial intelligence to boost (sub) seasonal forecasting

https://ai4s2s.readthedocs.io/

Apache License 2.0

20 stars 7 forks source link

Calendar backlog items #77

Closed BSchilperoort closed 2 years ago

BSchilperoort commented 2 years ago

This PR aims to close a few backlog issues about the Calendar/resampling functionality:

AI4S2S/s2spy#67
AI4S2S/s2spy#44
AI4S2S/lilio#10

List of changes:

Added check to the input data frequency. Warns the user if the input data frequency is lower than the calendar freq.
Added a check to see if intervals are missing after resampling (e.g. the data was for 2000 → 2008, while the calendar was mapped 2000 to → 2010). Warns the user if there are missing values (NaNs).
Input data checks are placed in ./utils.py to avoid cluttering _resample.py.
Added allow_overlap kwarg to allow overlapping intervals between anchor_years (and thus not skip years).

Closes AI4S2S/s2spy#67 Closes AI4S2S/s2spy#44

BSchilperoort commented 2 years ago

@Peter9192 , @geek-yang I have currently implemented allow_overlap as a kwarg for the calendars. However, this starts to make the amount of arguments for a calendar quite extensive (pylint complains, although it already counts 'self' as an argument in the max number).

An alternative way I can implement this would be by implementing a setter, e.g.:

calendar = AdventCalendar(anchor=(12, 31), freq='1d', max_lag=400)
calendar = calendar.set_allow_overlap(True)

geek-yang commented 2 years ago

@Peter9192 , @geek-yang I have currently implemented allow_overlap as a kwarg for the calendars. However, this starts to make the amount of arguments for a calendar quite extensive (pylint complains, although it already counts 'self' as an argument in the max number).

An alternative way I can implement this would be by implementing a setter, e.g.:
calendar = AdventCalendar(anchor=(12, 31), freq='1d', max_lag=400)
calendar = calendar.set_allow_overlap(True)

To me, it makes more sense to have this allow_overlap as a kwarg, since it is one of the choices that the user needs to make to have the desired calendar. The setter thingy sounds more logical for an added feature. I would prefer to keep it the way it is implemented now.