Skip to content
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

repeat() function #68

Open
oscarostlundgs opened this issue Oct 31, 2019 · 6 comments
Open

repeat() function #68

oscarostlundgs opened this issue Oct 31, 2019 · 6 comments

Comments

@oscarostlundgs
Copy link

Describe the problem.
Provide a repeat() function that repeats the last available values in a timeseries for missing date.

Describe the solution you'd like
repeat(x, mode) function takes a timeseries and a mode argument. The mode argument can be either CALENDAR_DAYS and it then repeats values on all calendar days or WORK_DAYS and then skips Saturdays and Sundays.

Describe alternatives you've considered
We considered having a UNION mode that would align the series with the date of another series and fill the missing values but that would be redundant as the align() already supports that use case.

Are you willing to contribute
Yes

@francisg77
Copy link
Contributor

Sounds good. One thought might be "mode" supporting a "calendar" parameter instead? We could make it just support a calendar day and work day calendar for now, but later could be for example an NYSE calendar or others?

@andrewphillipsn
Copy link
Contributor

Agreed, just specifying the calendar would work and simplify

@andrewphillipsn
Copy link
Contributor

should probably take this GsCalendar as second param?

@oscarostlundgs
Copy link
Author

Makes sense.
It seems the default constructor is a workday calendar.
Should we create constants for the Calendar day and Work day calendars?

@oscarostlundgs
Copy link
Author

We should reuse as much of the interpolate() code as possible. It would to support the other type of interpolations (e.g. linear) out of the box.

I can see two potential implementations:

  1. change the signature of interpolate() to also accept a calendar as a second argument (in addition to date arrays and timeseries)
    Pros: reduce number of functions, reuse code
    Cons: increases function complexity, though the function could be refactored, increases complexity of signature for users.

  2. add another wrapper function (e.g. series_fill() or interpolate_calendar()) that takes a timeseries and a calendar as arguments, constructs the date array based on the calendar and calls interpolate
    Pros: simpler function code, simpler function signature, good reuse of code
    Cons: increases number of functions

2 seems like a better approach but we need to carefully consider the naming of the function to make it self-explaining.

@francisg77
Copy link
Contributor

I'd lean towards interpolate() having the calendar parameter, otherwise in seeing the function you will wonder what the equivalent is with calendar support, and seems trickier to know what it should be called.

It can always be the final optional parameter too, so everyone doesn't have to specify.

Agree on having some constants too for the calendars (though constructor should take a string too, as likely don't want to make code changes for each new calendar).

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

No branches or pull requests

3 participants