ENH: Cleanup backend for Offsets and Period
State: closed

This PR is a work in progress

Fixes #5306 - Removing unused imports
Fixes #5082 - Added `inferred_freq_offset`
Fixes #5028 - fixed issues with `_offset_map`
Fixes #4878 - `freq` parameter for `Period` init supports full set of Offsets.
Fixes #5418 - raise `DateParseError` rather than incorrect `KeyError` for invalid frequencies


Labels:
name: Bug
name: Frequency
description: DateOffsets

Comments:
I know it seems weird, but feels like it would be much easier to just have DateOffset subclass str.

@jtratner While there may be benefits to having DateOffset subclass str, I don't think we can rely on the rulecode being == corresponding DateOffset even in the case where DateOffset subclass str.

no, it's not for eq, you'd override that anyways, it's more to not have to create 2 more properties (freq_obj, inferred_freq_offset) and a bunch of corresponding methods

That might work as long as you made sure to return the str value was always the canonical string.

For example you never got back 'Q' but rather instead got 'Q-MAR'.

whatever you're going to have for `freq_obj`, clearly you could just shift that to be returned by `freq`, and whatever `freqstr` you'd have on `freq_obj` would be what you'd want.  I don't really think a str subclass would work, it would just be a very elegant solution if it did.

I'm assuming you're going to change `freq` and `inferred_freq` to be just:

```
def freq(self):
     return getattr(self.freq_obj, 'freqstr', None)
def inferred_freq(self):
    return getattr(self.inferred_freq_offset, 'freqstr', None)
```

Currently freq and freqstr are written:

``` python
    # alias to offset
    @property
    def freq(self):
        return self.offset

    @property
    def freqstr(self):
        return self.offset.freqstr
```

I wrote this, but eventially this could be made more elegeant:

``` python
    @cache_readonly
    def inferred_freq_offset(self):
        try:
            return get_offset(self.inferred_freq)
        except ValueError:
            return None
```

@jtratner I am getting stuck now with the following:

creating a period requires a period "code" or "base" (an integer). This integer code is used by `tslib.period_ordinal` to convert the date to an ordinal. Unfortunately, `get_period_ordinal` is in cython which makes working with it difficult. What do you think of having some way of checking the `DateOffset` for an instance method "`get_period_ordinal`" before calling to cython? That way new `DateOffset` that want to handle their own conversion to periods can do so on the python class rather than having to add a new "period code" and then to add cython code?

The issue what I am trying to solve is taking data collected in 52-53 week fiscal years and up sampling to weeks. By design this should be a very clean upsampling (the quarters/years always end on the same day of the week).

@cancan101 what specifically are you having trouble with in regards to `get_period_ordinal`. Usually functions are written in cython for performance, so I'd prefer not to add a `hasattr` check anywhere.

I am still hacking around. Any hasattr will be done outside of any loop.
i.e. if it does end up needed, it will be called once per numpy array (not
per element)

On Wed, Oct 23, 2013 at 8:57 PM, Jeff Tratner notifications@github.comwrote:

> @cancan101 https://github.com/cancan101 what specifically are you
> having trouble with in regards to get_period_ordinal. Usually functions
> are written in cython for performance, so I'd prefer not to add a hasattrcheck anywhere.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/5148#issuecomment-26959204
> .

@cancan101 okay, well that was my quick response to your question. :smile: Ping me when you have something you'd like to ask about.  To be clear: we've already merged the 52-53 week offset, so why wasn't this an issue in that PR when it is an issue now?

There is no issue with the FY5253 et al. 

There are a number of pandas offsets which are not fully handled as periods. This DOES include the new 5253 offsets, but it also includes BQuarter (see #5091) and some others.

I am trying to clean some of that up.

@cancan101 okay - that's totally fine.

Can you move the location of the 52-53 week classes in the offsets.py file? When you rebased they ended up inserted between `QuarterOffset` and the various offsets that subclass `QuarterOffset`, makes it harder to read.

Sure. I will tack on to this pr.
On Oct 23, 2013 9:50 PM, "Jeff Tratner" notifications@github.com wrote:

> Can you move the location of the 52-53 week classes in the offsets.py
> file? When you rebased they ended up inserted between QuarterOffset and
> the various offsets that subclass QuarterOffset, makes it harder to read.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/5148#issuecomment-26961152
> .

great

@jtratner 

Do you think that if:
`pd.tseries.frequencies.get_offset(X) == pd.tseries.frequencies.get_offset(Y)`
then
`repr(pd.tseries.frequencies.get_offset(X))==repr(pd.tseries.frequencies.get_offset(Y))`
?

Nope, not going to be that way because of the special-case backwards-comptible reprs. Not a big deal.

I actually think it can be. Or in many more cases it can be with a very small change.

At least for:

```
    'QS': 'QS-JAN',
    'BQ': 'BQ-DEC',
    'BQS': 'BQS-JAN',
```

@jtratner If you have time, could you take a look? I have a first pass at making it easier to add new periods / correctly support existing periods without having to get into cython code.

@cancan101 in when you list the issues this closes, you need 'fixes issuexxx` each on a separate line (e.g. need fixes for each issue), in order to have automatic closures when this is merged

@cancan101 where are you on this? I think I looked it over earlier, do you want me to look through again?

Let me look over the changes so far and see if there are any remaining
todos on my side.

On Wednesday, November 27, 2013, Jeff Tratner wrote:

> @cancan101 https://github.com/cancan101 where are you on this? I think
> I looked it over earlier, do you want me to look through again?
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/5148#issuecomment-29421053
> .

@jtratner This looks ready to be reviewed. 

There still remain some pre-existing frequencies (for example the Business Years, Quarters and Months) for which `Pandas` does not have "`Period` support" (i.e. there is no corresponding logic for handling the respective `Period`. I don't think that needs to be solved now. This PR added the framework and structure (through `_NonCythonPeriod`) to made adding those `Period`s much easier down the road.

This PR looks promising. What's the status?

needs to be rebase

and a perf check 

@cancan101 also needs to be broken up into 2 or more PRs (that can be on top of each other or not)
for clarity

Okay this is rebased.

Is it okay for me to squash this down to one commit?

@cancan101 can u annotate the top section to what each issue is that is being fixed

This is the most pressing performance regression:

```
period_setitem                               | 285.7073 | 162.4957 |   1.7582 |
```

which seems to be caused by an extra call (the new new code adds one more call) to `get_freq_code` aka `_gfc` in the `Period` constructor.

I am looking into either removing the extra call, speeding up `gfc` or perhaps memoizing the results (I would assume this is frowned upon).

It appears that ~44% of the time is spent in `com.is_integer`. I have filed #6264 to fix this.