BUG: TDI.insert with empty TDI raising IndexError by jbrockmendel · Pull Request #30757 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits10 Checks0 Files changed
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 }})
This started out as a cosmetic-only branch and ended up finding a broken corner case. The relevant change is in timedeltas L416 where if self.freq is not None: is now if self.size and self.freq is not None:
Using _check_compatible_with causes us to raise TypeError instead of ValueError in a couple of the DatetimeIndex.insert cases.
mypy 0.730
Performing static analysis using mypy
pandas/core/indexes/timedeltas.py:56: error: Definition of "_data" in base class "ExtensionIndex" is incompatible with definition in base class "DatetimelikeDelegateMixin"
pandas/core/indexes/timedeltas.py:56: error: Definition of "_data" in base class "Index" is incompatible with definition in base class "DatetimelikeDelegateMixin"
pandas/core/indexes/datetimes.py:87: error: Definition of "_data" in base class "ExtensionIndex" is incompatible with definition in base class "DatetimelikeDelegateMixin"
pandas/core/indexes/datetimes.py:87: error: Definition of "_data" in base class "Index" is incompatible with definition in base class "DatetimelikeDelegateMixin"
Found 4 errors in 2 files (checked 844 source files)
@simonjayhawkins suggestions for fixing mypy?
am AFK for the rest of the day (Pydata London). will look tomorrow.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some possible followup comments, otherwise lgtm.
| @@ -96,7 +96,7 @@ class DatetimeIndexOpsMixin(ExtensionIndex, ExtensionOpsMixin): |
|---|
| Common ops mixin to support a unified interface datetimelike Index. |
| """ |
| _data: ExtensionArray |
| _data: Union[DatetimeArray, TimedeltaArray, PeriodArray] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should define this with a short label, maybe DatetimeArrayType
| @@ -946,7 +945,7 @@ def insert(self, loc, item): |
|---|
| freq = self.freq |
| elif (loc == len(self)) and item - self.freq == self[-1]: |
| freq = self.freq |
| item = _to_M8(item, tz=self.tz) |
| item = item.asm8 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove _to_M8?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove _to_M8?
IIRC there's still one more usage to get rid of after this. I'm eager to get rid of it, as im pretty sure it is hiding a bunch of corner case bugs.
| idx[:0].insert(0, td) |
|---|
| idx[:0].insert(1, td) |
| idx[:0].insert(-1, td) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe worth comparing the results
