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
Resample as a method vs. as a function. #50
Comments
Yep, it would focus the AdventCalendar to be responsible for just making intervals and not for resampling. Setting the calendar variable again changes the type from import s2spy.time
calendar = s2spy.time.AdventCalendar()
intervals = calendar.map_years(2010, 2020)
data_resampled = s2spy.time.resample(intervals, data) |
This is actually handled by the The old functionality indeed used to be that map_years returned intervals. However, if we do not need to use any additional methods on the calendar, there is no clear reason to store them like this. |
Thanks for the discussion. The calendar is meant for a feedback to the user. It is not used for calculation. So I think the current way that we save intervals to But I agree that we just attach too many functionalities to the We can further discuss it when the team is back and re-organize the time module. |
I very much support the proposal of making resample a separate function. With respect to the reuse of variables, I agree with Stefan as well. I think people would typically do this in one go: calendar = s2spy.time.AdventCalendar().map_years(2010, 2020) and I guess you could argue that the AdventCalendar class should actually be an AdventCalendarBuilder class or something like that. But I'm not so well versed in design patterns, so I might be wrong here (see https://refactoring.guru/design-patterns/builder). |
During today's demo session @sverhoeven asked why
resample
is a calendar method, versus just a function that takes a calendar and data as input.After some consideration I do agree that this could be a more clear way to do it, and it would demarcate the functionalities of the calendar a bit better (instead of putting everything in it as a method).
This issue is related to #41.
Current implementation:
Alternative implementation:
The text was updated successfully, but these errors were encountered: