API: require timezone match in DatetimeArray.shift by jbrockmendel · Pull Request #37299 · pandas-dev/pandas (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

i guess this is a small api change (e.g. what did this do before)?

can you add a whatsnew note (rebase as well)

@jbrockmendel what's the rationale of requiring matching timezone here? (as long as tz-awareness matches)

what's the rationale of requiring matching timezone here? (as long as tz-awareness matches)

Trying to have some consistency about which methods require matching tz vs just tzawareness-compat. The heuristic is to require matching tz for setitem-like methods.

Isn't that inconsisten with other operations like aritmetic/comparison ops, which allow it? (like the subtract PR that is also open)

Also setitem on Series doesn't actually raise, so on the Series level, those methods are also not fully consistent.

Isn't that inconsisten with other operations like aritmetic/comparison ops, which allow it? (like the subtract PR that is also open)

Right, those methods are not considered "setitem-like". searchsorted is also considered comparison-like as opposed to setitem-like.

Also setitem on Series doesn't actually raise, so on the Series level, those methods are also not fully consistent.

The Series is cast to object and the value being set is not cast to the existing timezone. DatetimeBlock._can_hold_element is equivalent to

def _can_hold_element(self, element):
    try:
        self.array_values()._validate_setitem_value(element)
        return True
    except TypeError:
        return False

Isn't that inconsisten with other operations like aritmetic/comparison ops, which allow it? (like the subtract PR that is also open)

Right, those methods are not considered "setitem-like". searchsorted is also considered comparison-like as opposed to setitem-like.

Also setitem on Series doesn't actually raise, so on the Series level, those methods are also not fully consistent.

The Series is cast to object and the value being set is not cast to the existing timezone. DatetimeBlock._can_hold_element is equivalent to

def _can_hold_element(self, element):
   try:
       self.array_values()._validate_setitem_value(element)
       return True
   except TypeError:
       return False

we should rationalize these one way or the other for all ops. I would be +1 on always being strict here, though this would break some existing code / tests.

we should rationalize these one way or the other for all ops. I would be +1 on always being strict here, though this would break some existing code / tests.

I'd be on board with being consistent across all these methods, but I think we would basically have to make it the non-strict version. Otherwise we would break consistency with the stdlib datetime on comparisons (xref #37329).

thanks @jbrockmendel

yeah I agree we need to make this consitent, but this PR itself is fine as its just requires a tz matching fill value.

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request

Nov 2, 2020

ukarroum pushed a commit to ukarroum/pandas that referenced this pull request

Nov 2, 2020

Labels