COMPAT: Scalar types immutable

[jbrockmendel]

My impression (discussion somewhere on gitter) is that pd.Period is supposed to be immutable. That would certainly make sense for objects going into a PeriodIndex.

per = pd.Period('2014Q1')
>>> per
Period('2014Q1', 'Q-DEC')

freq = per.freq.copy()
freq.n *= 2
per.freq = freq
>>> per
Period('2014Q1', '2Q-DEC')

per.freq is a DateOffset object*, which is defined in tseries.offsets. There is a comment in tseries.offsets.BusinessHourMixin.apply saying "# calculate here because offset is not immutable".

So a couple of questions:

1. Are we sure pd.Period should be immutable? If so, is there a preferred way of imposing that? Note that the class is defined in a .pyx file and some methods explicitly call __new__.
2. Does the comment about offset not being immutable mean that it can't or shouldn't be? Outside of __init__ and __setstate__, the only place in tseries.offsets where I see any self.foo attributes being set is self.daytime = False in BusinessHourMixin._get_business_hours_by_sec. grepping for "daytime", this attribute does not appear to be referenced anywhere else.

* BTW, I think DateOffset is one of a small number of commonly-used classes for which there is not a ABCDateOffset equivalent. Is that by design? If not, I'll be happy to make one.

[shoyer]

Yes, it sure seems like pd.Period should be immutable, though I don't know the historical context.

If so, is there a preferred way of imposing that?

The standard way is to hide attributes like freq behind properties, so they can only be accessed through public APIs, not set, e.g.,

class Period(object):
    ...

    @property
    def freq(self):
        return self._freq

[jbrockmendel]

OK, I can take care of this.

As long as we're looking at futzing with pd.Period, this might be a good time to make design decisions about _libs.period.IncompatibleFrequency. See e.g. #17112, #13129, #15183, ...

Right now two pd.Period objects can be compared (sorted, put into a well-behaved Index,...) if and only if they have the same frequency. AFAICT this choice was made in order to avoid having to take a stand on the other cases. How about a lexicographic ordering based on (start_time, freq)? Timestamp and datetime objects can implicitly have a frequency of zero.

[shoyer]

@jbrockmendel Comparisons between periods with different frequencies is also worth discussing, but please open a separate issue for that.

[jbrockmendel]

In profiling some downstream code I found that DateOffset.__ne__ and DateOffset.__eq__ were taking an enormous amount of time. It looks like this is because it is calling self._params() == other._params() in each call to __eq__. Point being: there's some real mileage to be picked up by making _params() a cache_readonly or something.

[jreback]

Note that we don't explicity test, nor write down anywhere, the implicit guarantee for the pandas scalars Timestamp/Period/Interval that they are immutable.

So most important are tests here.

This is almost trivial to fix. See how _Timestamp is defined, IOW the attributes are cdef readonly, while Period are not.

Would appreciate tests on all of the scalar types.

[jbrockmendel]

This is almost trivial to fix. See how _Timestamp is defined, IOW the attributes are cdef readonly, while Period are not.

Current definition:

cdef class _Period(object):

    cdef public:
        int64_t ordinal
        object freq

So just change "cdef public" to "cdef readonly"?

Would appreciate tests on all of the scalar types.

I'm happy to help, but am stretched pretty thin. For now I need to triage.

[gfyoung]

So just change "cdef public" to "cdef readonly"?

Yes, that is correct.

I'm happy to help, but am stretched pretty thin. For now I need to triage.

No worries! This is not urgent, so feel free to take your time on this.

[jbrockmendel]

What about for the non-cython DateOffset? It's got a kwds attribute that's a dict.

[jbrockmendel]

Changing ordinal and freq to cdef readonly breaks Period._from_ordinal, which is called by Period.__init__.

    @classmethod
    def _from_ordinal(cls, ordinal, freq):
        """
        Fast creation from an ordinal and freq that are already validated!
        """
        if ordinal == iNaT:
            return NaT
        else:
            self = _Period.__new__(cls)
            self.ordinal = ordinal
            self.freq = cls._maybe_convert_freq(freq)
            return self

Please advise.

[gfyoung]

@jbrockmendel : I think what you should do is try to see which attribute you can make readonly without breaking anything. For those that do, try to refactor so that you can initialize as many of these parameters as possible. I can't see the code ATM to judge how feasible this second sentence is, but I hope that should help for the time being.

[jbrockmendel]

There are only two _Period attributes that are relevant: ordinal and freq. Both of them are currently declared public, and both of them are set from within the classmethod Period._from_ordinal, which is called at the end of Period.__init__.

[gfyoung]

Can you initialize them in __new__ ? Otherwise, it might be difficult to make these readonly otherwise.

[jbrockmendel]

Correction: _from_ordinal is a classmethod of _Period, not Period. Shouldn't make a difference, but be advised.

Can you initialize them in __new__ ?

__new__ and cython are both at the edges of my comfort zone. I'm not the person we want futzing with this. That said...

Changing _from_ordinal to look like:

    @classmethod
    def _from_ordinal(cls, ordinal, freq):
        """
        Fast creation from an ordinal and freq that are already validated!
        """
        if ordinal == iNaT:
            return NaT
        else:
            freq = cls._maybe_convert_freq(freq)
            self = _Period.__new__(cls, ordinal=ordinal, freq=freq)
            # self = _Period.__new__(cls)
            # self.ordinal = ordinal
            # self.freq = cls._maybe_convert_freq(freq)
            return self

Then the constructor "works", but...

>>> per = pd.Period('2016Q1', freq='Q')
>>> per
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pandas/_libs/period.pyx", line 952, in pandas._libs.period._Period.__repr__ (pandas/_libs/period.c:17168)
    base, mult = frequencies.get_freq_code(self.freq)
  File "pandas/tseries/frequencies.py", line 347, in get_freq_code
    base, stride = _base_and_stride(freqstr)
  File "pandas/tseries/frequencies.py", line 587, in _base_and_stride
    groups = opattern.match(freqstr)
TypeError: expected string or buffer

Also freq and ordinal are set inside __setstate__. No idea what to do about that.

[gfyoung]

@jbrockmendel : I know you said you were stretched thin logistically, so this is great that you've been able to dive into this refactoring with some detail. No worries if you're stuck. At least people will know where to pick things up for the time being.

[jbrockmendel]

Possibly closed by #21341

[gfyoung]

@jbrockmendel : Would you be able to confirm? Can we add tests to check?

[jbrockmendel]

@gfyoung The original motivations (Period and DateOffset) have been made immutable. The part I'm not sure about is if there were other scalars @jreback had in mind when making the issue title more general.

[jbrockmendel]

Closed by #17239 and #21341