Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: require timezone match in DatetimeArray.shift #37299

Merged
merged 11 commits into from
Oct 31, 2020

Conversation

jbrockmendel
Copy link
Member

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

@jreback jreback added the Timezones Timezone data dtype label Oct 22, 2020
@jreback jreback added this to the 1.2 milestone Oct 22, 2020
@jreback jreback added the Error Reporting Incorrect or improved errors from pandas label Oct 22, 2020
@jreback
Copy link
Contributor

jreback commented Oct 22, 2020

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

can you add a whatsnew note (rebase as well)

@jorisvandenbossche
Copy link
Member

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

@jbrockmendel
Copy link
Member Author

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.

@jorisvandenbossche
Copy link
Member

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.

@jbrockmendel
Copy link
Member Author

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

@jreback
Copy link
Contributor

jreback commented Oct 26, 2020

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.

@jbrockmendel
Copy link
Member Author

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).

@jreback jreback merged commit f7c3dd2 into pandas-dev:master Oct 31, 2020
@jreback
Copy link
Contributor

jreback commented Oct 31, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas Timezones Timezone data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants