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

Rebase BaseCalendar to CustomCalendar #122

Closed geek-yang closed 1 year ago

geek-yang commented 1 year ago

Replace BaseCalendar with CustomCalendar, which includes following steps:

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

geek-yang commented 1 year ago

Hi Yang, good job! I did find some small bugs, but that's expected with such a big overhaul!

I noticed that the tutorial_resampling_data is not updated, and does not work at the moment (anchor definition is incorrect). Can you update that for this PR?

Also, I am not a big fan of splitting the calendar explanations over multiple notebooks. How about just two? One for the Advent/Monthly/Weekly calendars, and one for the CustomCalendar? Or one main one using only the Advent calendar, and then a notebook about alternatives?

Thanks for the review! 😂 I forgot the tutorial_resampling_data. Thanks for noticing this. I will fix it.

And I like your idea about only keeping the adventcalendar notebook and merge the others as one alternative_calendar notebook. I will implement this. 👍

geek-yang commented 1 year ago

For the other tests (e.g. test_resample.py), it is not relevant with this one and let's fix them in next PR, where I would like to fix traintest and RGDR.

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

88.0% 88.0% Coverage
0.0% 0.0% Duplication