ENH: Experimental CustomBusinessDay DateOffset class. fixes #2301
State: closed

I took a stab at issue #2301 using the busdaycalendar functionality in numpy 1.7.
### Caveats

There are a few issues with this:
- It requires Numpy 1.7 (I've only tested it on numpy 1.7.1).
- The timezone handling in nump 1.7 is broken (according to [this discussion](http://numpy-discussion.10968.n7.nabble.com/timezones-and-datetime64-td33407.html) ) and the datetime64 and timezone API will change in numpy 1.8. So from what I understand the current code as I've written it might move you onto a different day if you're in a UTC-0?00 timezone (I'm in UTC+0200 so it didn't affect me).

That said I didn't want to reinvent the wheel and since the numpy datetime64 api does cater for this usecase I thought it would be good to standardise on that in the long term.

The code does what I need it to do and I'm putting it out there in case it's useful to anyone else. Also, with some feedback maybe we can get it to the point where it could be included in Pandas as an optional DateOffset class for users who have Numpy 1.7.
### Possible Improvements

I can think of:
- Guard in the constructor to raise a meaningful exception when the user doesn't is on Numpy < 1.7.
- Better handling for the timezone issue.
- Unit tests for the CustomBusinessDay behaviour.
### Notes
- I picked the frequency code 'C' because it was available and fit nicely between 'D' and 'B'. I had originally named the class TradingDay and wanted to use 'T' but that's already used for Minutes.
- I have no idea what goes on in frequencies.py and I simply put a 'C' wherever I found a 'B' using the reasoning that as a BusinessDay subclass it should work in all the same places.


Labels:

Comments:
need some tests for this....

can you add a release notes metion / v0.11.1 mention, and a doc mention (timeseries.rst) , mark as experimental

(e.g. you can do in the heading)

also need some tests......

Ok, thanks for the pointers I will try to work on this today.

can you hook up to travis (which this should fail on several versions as numpy < 1.7)

and raise an error if numpy < 1.7 (can do in **init** method)

and skip tests for same...

thxs

Ok. I'm busy reading the Travis-CI section in CONTRIBUTING.md ...

great! let me know if its not clear

Not sure why the nose.SkipTest is causing a NameError on some of the Travis builds because I copied the code from _skip_if_no_pytz() and that appears to be working. My best guess is that it doesn't work in the setUp() method so I'm moving it out into the test_*() methods.

correct

just do it in each test (a bit annoying)
other way is to use a separate testing class and just conditionally create it

I think you might be able to skip the tests in one go with a try/catch statement (I think):

``` python
try:
    import pytz
except ImportError:
    MyClass = nose.tools.nottest(MyClass)
```

Better would probably be to move it to a separate test file with this at the top:

``` python
try:
   import pytz
except ImportError:
   raise nose.SkipTest
```

Is it maybe due to me passing a message argument to nose.SkipTest?

I'm going to look at this in the morning - it's getting late here.

Did you import nose? That is what would cause a nameerror (or trying to use
Skip Test before importing it)
On Jun 19, 2013 5:13 PM, "Tobias Brandt" notifications@github.com wrote:

> Is it maybe due to me passing a message argument to nose.SkipTest?
> 
> I'm going to look at this in the morning - it's getting late here.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/3744#issuecomment-19715424
> .

@jtratner No, I don't import nose and I saw that it wasn't imported anywhere. I initially wondered about that but there is some other code in the same module that also uses it and which appears to run fine so I concluded that nose must just be made globally available when running nosetests. Is that not the case?

It could be that the nose.SkipTest in _skip_if_no_pytz() is never actually run if pytz is always available in all the Travis build environments in which case I was misled by other faulty code that just doesn't get called.

You have to import nose :) if you see places that don't they are wrong.
On Jun 19, 2013 5:20 PM, "Tobias Brandt" notifications@github.com wrote:

> @jtratner https://github.com/jtratner No, I don't import nose and I saw
> that it wasn't imported anywhere. I initially wondered about that but there
> is some other code in the same module that also uses it and which appears
> to run fine so I concluded that nose must just be made globally available
> when running nosetests. Is that not the case?
> 
> It could be that the nose.SkipTest is never actually run if pytz is always
> available in all the Travis build environments.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/3744#issuecomment-19715883
> .

And if you try it again I'm guessing you can raise it in setup (though
setup class might be the better choice)
On Jun 19, 2013 6:06 PM, "Jeffrey Tratner" jeffrey.tratner@gmail.com
wrote:

> You have to import nose :) if you see places that don't they are wrong.
> On Jun 19, 2013 5:20 PM, "Tobias Brandt" notifications@github.com wrote:
> 
> > @jtratner https://github.com/jtratner No, I don't import nose and I
> > saw that it wasn't imported anywhere. I initially wondered about that but
> > there is some other code in the same module that also uses it and which
> > appears to run fine so I concluded that nose must just be made globally
> > available when running nosetests. Is that not the case?
> > 
> > It could be that the nose.SkipTest is never actually run if pytz is
> > always available in all the Travis build environments.
> > 
> > —
> > Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/3744#issuecomment-19715883
> > .

Ok, all builds pass now with the exception of one where there is one Error. However the remaining error is in test_pytables.py which I didn't touch. I must have based off a bad version of master (wasn't using Travis at the time).

I will look at the other suggestions tomorrow.

@snth I think u are based off of a somewhat older version of master

try:

git checkout master
git branch your_branch --set-upstream master
git pull
git checkout your_branch
git pull --rebase

then fix any merge conflicts

Won't rebasing now cause problems because I already pushed to my origin?

I did a test merge in a new branch yesterday and there were only two conflicts in the release docs which I can resolve.

I'm not sure whether to rebase or merge now?

@snth don't `merge`, you want to `rebase` otherwise it adds unnecessary merge commits into the history.  If you're not sure about whether your master is on the same track as the main pandas repo, you might want to rebase off of pydata/pandas/master instead of your own master branch, like this:

``` bash
git remote add upstream https://github.com/pydata/pandas
git fetch upstream
git rebase upstream/master
```

As an aside I have `git uprb` aliased to the 2nd and 3rd command above (snippet from my .gitconfig):

``` bash
[alias]
  uprb = "!git fetch upstream && git rebase upstream/master"
```

I've got pydata/pandas set up as upstream and just did a rebase off upstream/master in a test-branch and it was fine. Just the general git advice on the internet is that you shouldn't rebase after you've pushed to another repo so I'm unsure what's going to happen to this pull-request once I rebase this branch?

push appends
rebase rewrites

the advice not to rebase ONLY matters is if u are collaborating with other folks ON THE SAME REPO
but you push to your own repo

so go ahead and rebase

it's use to clean things up 

in addition since master has changed since u originally cloned it
your fixes might not work until we try to merge them to master
much more helpful for u 2 work with an up to date copy

Ok, just did `git rebase master` and `git push -f origin custom-business-day` as `git push origin` failed.

you can't push to origin, only your branch

next need u to squash some of these commits

so

git rebase -i <start commit>

then s some of them

just combine ones that make sense

then push -f again

@jreback I added `holidays` and `weekmask` arguments to `cdate_range` as per your suggestion. I handled everything right inside `CustomBusinessDay` because the logic in `DatetimeIndex.__new__` was quite involved and I'd rather not risk messing anything up there.

great...

if one does not pass `holidays`, but uses `C`....should this just return a `B` freq? (I am not sure, maybe too much magic, though it does make sense)

is holidays accessible from the freq (`.holidays`)? can you change it (I think not)

What about combing CBusiness days of different holiday schemes? need a test for this (I think its ok if there holidays are `==`, otherwise lose the freq) - I am pretty sure that Business freq does this when you say combine with a `D` freq), e.g. `bdate_range(....,freq='B') + date_range(....freq='D')`

hmmm...this look right to u?

```
In [4]: pd.bdate_range('20130101',periods=5) + pd.date_range('20130110',periods=5)
Out[4]: 
<class 'pandas.tseries.index.DatetimeIndex'>
[2013-01-01 00:00:00, ..., 2013-01-14 00:00:00]
Length: 10, Freq: B, Timezone: None
```

I'm not sure what the expected behaviour under DatetimeIndex.**add** is. I noticed that DatetimeIndex.equals() seems to compare the dates and ignores `freq` (this observation is based on behaviour without looking at the code). I in fact used this in the last batch of unit tests that I added. 

The example you posted looks like a bug since the result isn't the same as `bdate_range('20130101', periods=10)`. Seems like DatetimeIndex.**add** acts like union but the `freq=='B'` should be dropped.

In terms of the `.holidays` and `.weekmask` attributes on `CustomBusinessDay`, I agree that it would be nice to have these as properties and any changes should be detected and cause the `busdaycalendar` to be updated. However this operation would invalidate anything that is marked as `freq=='C'` which raises the bigger issue of how to establish equality for parameterised DateOffsets like this? In my use case I just needed it to show up with any frequency at all and I make sure on my side that 'C' has a unique meaning everywhere. Given that this feature is very clearly marked 'experimental', do you think we can leave it at 'use at your own risk' for now?

I'm sorry I can't commit any more time to this until the weekend and even then it's not likely that I'll get to it. Any chance we could open separate issues for these? I can squash the commits together but then have to move on.

@wesm ok as experimental?

@snth go ahead and rebase to a smaller num of commits...

I've squashed bug-fix commits and kept the incremental build up of code. Let me know if that's still too verbose and you want it squashed further. Travis is happy so good to go if you guys are happy.

@snth I'd squash all the ENH commits together and all the TST commits together. [I might do it in two passes: first, reorder TST and ENH to be next to eachother, second, squash ENH and TST] If you use squash, your notes will still be there, but in terms of descriptiveness, those are all working together to get to one common goal - so it's potentially more helpful to have them together (i.e., if I look at your big ENH commit, it's clear exactly what's going into it. But that's not an official opinion :)

to be clear - I mean squash separately (so one ENH commit and one TST commit)