Tslibs offsets immutable
State: closed

This is the big hurdle towards making DateOffsets immutable.

- Make tslibs.offsets._BaseOffset a cython class, define `n` and `normalize` to be readonly
- Update tests that try to alter offsets inplace
- Poke at things until pickle tests stop failing <-- the part with the kludge

I'm not wild about the hack in pickle_compat, but haven't found any other way to avoid `TestCommon.test_pickle_v0_15_2` failing with:

```
cls = <class 'pandas.tseries.offsets.YearBegin'>, base = <type 'object'>, state = None

    def _reconstructor(cls, base, state):
        if base is object:
>           obj = object.__new__(cls)
E           TypeError: object.__new__(YearBegin) is not safe, use pandas._libs.tslibs.offsets._BaseOffset.__new__()
```

After this there will then be a) a bunch of other attributes to make readonly and b) the optimizations that are available once DateOffsets are immutable.  But first we need to either OK the pickle_compat hack or find another way around.

Labels:
name: Internals
description: Related to non-user accessible pandas implementation
name: Timedelta
description: Timedelta data type

Comments:
this is not a feasible work around. you will have to look how I handled the ndarray subclassing case (this is similar).
> this is not a feasible work around. you will have to look how I handled the ndarray subclassing case (this is similar).

OK, I'll keep poking at it.  Where did the ndarray subclassing occur?  Best guess is IndexOpsMixin?
Once we get #18224 sorted out, the `assert n == int(n)` will be done only once (and probably as a ValueError instead of assertion) in `_BaseOffset.__init__`

<b>Update</b> Woops, this comment belongs in #18226.
Will wrk be m this locally, get it off reviewers plate for now. Closing.
I'm not having much luck with this, have not given up.  One idea: could we drop support for the 0.15.2 pickle?  That would solve the current problem.
Implementing `__reduce__` just leads to _more_ errors.  Can you expand a bit on what the problem is with the approach in this PR via pickle_compat?
@jreback suggestions on who I might be able to ask for help on this?  Making offsets immutable is a pretty high priority for me (major PeriodIndex perf implications) and this is dead in the water.
@jbrockmendel not sure what is the specific issue
> not sure what is the specific issue

TLDR: cython class + pickle + back-compat

We need to make `DateOffset`s immutable.  To get there we need to change `class _BaseOffset(object)` to `cdef class _BaseOffset(object)`.  But doing that leads to failure in `test_pickle_v0_15_2`.  The only [workaround](https://github.com/pandas-dev/pandas/pull/18224/files#diff-a5875a15c6438ff1f03e8d7294c03deaR50) I've come up with got shot down.
I actually don’t have a problem with breaking pickles for offsets; you wooild rarely pickle these anyways

but i think u can still do this by using our substitution machinery - why don’t i give this a try
> but i think u can still do this by using our substitution machinery - why don’t i give this a try

Awesome, go for it!  My attempt to use the substitution machinery didn't work out, so with a little luck I'll learn something from seeing the results.

> I actually don’t have a problem with breaking pickles for offsets

If the substitution approach doesn't work out, then after my remaining outstanding PRs are closed out I'll do it this way.
> why don’t i give this a try

@jreback gotten a chance to look at this?  I've been trying periodically and still haven't come up with anything other than the hack in `pickle_compat` here.
@jbrockmendel I couldn't pull down your branch locally (deleted?) but maybe this article helps:

https://stackoverflow.com/questions/4028321/any-ideas-about-the-best-work-around-for-new-losing-its-arguments

Namely that "You can't use object as the terminating class for your inheritance if you want to pass arguments to `__new__` or `__init__`." - perhaps explicitly defining `__new__` for `_BaseOffset` would help?
> I couldn't pull down your branch locally (deleted?)

Yah, looks like I deleted the branch.  I'll see if I can restore it without creating a new PR.

> perhaps explicitly defining `__new__` for `_BaseOffset` would help

Nope, cython raises a compile-time error telling you to use `__cinit__` instead of `__new__`.  If we do that, then in the tests we get errors in unpickling (see OP for description of the error and the kludged workaround).  See @jreback's response disapproving and suggesting I look at [I never figured out exactly what]
@WillAyd the branch should be ~restored (made a new branch with the same name, copy/pasted the diff).
Still trying to recreate from the branch provided and haven't seen through yet but wondering at the moment if even just making a pass-through `cinit` would work, ala:

```python
    def __cinit__(self, *args):
        pass
```

My current thinking there is a result of the below link:

http://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#initialisation-methods-cinit-and-init

Namely "If you anticipate subclassing your extension type in Python, you may find it useful to give the __cinit__() method * and ** arguments so that it can accept and ignore extra arguments. Otherwise, any Python subclass which has an __init__() with a different signature will have to override __new__() [1] as well as __init__(), which the writer of a Python class wouldn’t expect to have to do."

Doing such did resolve the TypeError you mentioned originally, though some other errors popped up I haven't had time to look at and may or may not have been resolved by subsequent work you did. Sharing for now in case this finding helps you at all
> Still trying to recreate from the branch provided and haven't seen through yet

Thanks for taking a look.  Note that when I put this together yesterday I got a test error in something like `test_offsets.test_pickle_v_0_15_2` that wasn't present at the time of the PR.  That is somewhat secondary, as The Problem is what happens when you revert the edits made in `pickle_compat`.


I don't get any errors when reverting the changes made in `pickle_compat` from the branch you remade
Not even in `TestCommon.test_pickle_v0_15_2`?  I'm getting that error (py27, trying py36 shortly) regardless of whether the pickle_compat changes are reverted or not.
No errors at all running

```bash
pytest pandas/tests/io/test_pickle.py
```

This was using python3 so maybe that's the differentiator
Hmm I've fixed the v0_15_2 error I was getting (had mixed and matched diffs), but I'm still getting failures when I revert the pickle_compat edits (py27 and py36, cython 0.28.3).