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

Merge cdate_range functionality into bdate_range #17596

Closed
jschendel opened this issue Sep 19, 2017 · 5 comments · Fixed by #17691
Closed

Merge cdate_range functionality into bdate_range #17596

jschendel opened this issue Sep 19, 2017 · 5 comments · Fixed by #17691
Labels
Milestone

Comments

@jschendel
Copy link
Member

Following up on some discussion from #17554.

The main point of discussion was regarding merging the functionality of cdate_range into bdate_range. This would involve adding the two additional keywords supported by cdate_range to bdate_range (weekmask and holidays), which would only be used (if provided) when a custom frequency is passed to bdate_range.

This wouldn't be terribly invasive, as cdate_range is only top-level on master (not 0.20.3), and there is no documentation mentioning it. Performing the merge would help keep the top-level API clean, and reduce the number of date_range related functions. On the user end, the change would amount to cdate_range(...) -> bdate_range(..., freq='C').

An alternative would be to merge both bdate_range and cdate_range into date_range. The only difference between bdate_range and date_range is the default value of freq, so from a technical standpoint it wouldn't add complexity to implement compared to just merging cdate_range -> bdate_range.

This would be a more invasive change though, as bdate_range currently is currently top-level, and there is some documentation mentioning it, so there'd be some additional work in terms of deprecation/doc modification. It would provide the advantage of having only one date_range related function. On the user end, the change would amount to cdate_range(...) -> date_range(..., freq='C'), and bdate_range(...) -> date_range(..., freq='B').

The current plan is to do the cdate_range -> bdate_range merge, but we're interested in community input.

cc @jorisvandenbossche

@jreback
Copy link
Contributor

jreback commented Sep 20, 2017

I would deprecate bdate_range. We don't really have much data on usage patterns, but I think being quite specific as the frequency is not much of a burden (IOW specifying 'B'), having a whole function to do this is just polluting the api.

I am -0 on keeping cdate_range. I think you could easily add these params to date_range (holidays and weekmask). It provides a bit more error checking to have it in a single functon (cdate_range) though.
sure the

@jorisvandenbossche jorisvandenbossche added this to the 0.21.0 milestone Sep 20, 2017
@jbrockmendel
Copy link
Member

FWIW the offsets.CustomFooOffset classes are much less robust than the others, will be the last to be ported into cython and made immutable. The offsets.BusinessFoo classes are better, but still less solid than the more basic classes.

@jorisvandenbossche
Copy link
Member

I would deprecate bdate_range. ... I am -0 on keeping cdate_range

If we deprecate something, I would rather do cdate_range and keep bdate_range. Reasons: 1) both are business-related, so merging their functionality seems logical 2) bdate_range is already top-level, public, documented for a long time, cdate_range not.
We could indeed also add the functionality to date_range, one downside for that is that it then gets even more keywords. So for now I would go for "cdate_range -> merge in bdate_range"

@jreback
Copy link
Contributor

jreback commented Sep 23, 2017

ok let's do it as described here (cdate deprecated and merged to bdate)

@jreback
Copy link
Contributor

jreback commented Sep 23, 2017

quite a bit of docs will need updating in timeseries.rst; could take opportunity to freshen those as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants