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

TYP: DataFrame.(index|columns) and Series.index #31126

Merged
merged 3 commits into from Jan 24, 2020

Conversation

topper-123
Copy link
Contributor

Makes mypy discover the .index and .columns attributes and that they're Index (sub-)classes.

This is done by manually adding the properties.AxisProperty to DataFrame/Series instead of doing it programatically, as mypy doesn't like adding attributes programatically very much.

@topper-123 topper-123 changed the title Type: DataFrame.(index|columns and Series.index Type: DataFrame.(index|columns) and Series.index Jan 18, 2020
@jreback jreback added the Typing type annotations, mypy/pyright type checking label Jan 18, 2020
@jreback jreback added this to the 1.1 milestone Jan 18, 2020
index: "Index" = properties.AxisProperty(
axis=0, doc="The index (axis labels) of the Series."
)
generic.NDFrame._internal_names_set.add("index")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this actually be on Series._internal_names_set? (and same for Frame)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, changed.

@@ -295,6 +298,9 @@ def _get_footer(self) -> str:
footer = ""

if getattr(self.series.index, "freq", None) is not None:
assert isinstance(
self.series.index, (ABCDatetimeIndex, ABCPeriodIndex, ABCTimedeltaIndex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we have a ABCDatetimelike ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, and I only found two instances of this pattern.

@topper-123 topper-123 changed the title Type: DataFrame.(index|columns) and Series.index TYP: DataFrame.(index|columns) and Series.index Jan 18, 2020
@@ -5300,8 +5304,10 @@ def reorder_levels(self, order, axis=0) -> "DataFrame":
result = self.copy()

if axis == 0:
assert isinstance(result.index, ABCMultiIndex)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this is a breaking change?

this PR

>>> pd.DataFrame().swaplevel()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\simon\pandas\pandas\core\frame.py", line 5270, in swaplevel
    assert isinstance(result.index, ABCMultiIndex)
AssertionError
>>>

on master

>>> pd.DataFrame().swaplevel()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\simon\pandas\pandas\core\frame.py", line 5268, in swaplevel
    result.index = result.index.swaplevel(i, j)
AttributeError: 'Index' object has no attribute 'swaplevel'
>>>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've made it a TypeError, to mirror reorder_levels. I also added a Whatsnew note.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, is this an API change? We're reaching 1.0, so in principle we should be wary on changing the API. Should I raise an AttributeError instead of TypeError to maintain compatibility (though that is strictly a wrong error to raise)?

@@ -379,7 +379,7 @@ def _generate_marginal_results_without_values(
):
if len(cols) > 0:
# need to "interleave" the margins
margin_keys = []
margin_keys: Union[List, Index] = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be ArrayLike? (not sure it matters, but can update in followon if this works)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would not work, because Union[List, Index] is the precise type of this.

@@ -666,7 +666,8 @@ Other API changes
- When testing pandas, the new minimum required version of pytest is 5.0.1 (:issue:`29664`)
- :meth:`Series.str.__iter__` was deprecated and will be removed in future releases (:issue:`28277`).
- Added ``<NA>`` to the list of default NA values for :meth:`read_csv` (:issue:`30821`)

- a ``TypeError`` is now raised if attempting to call :meth:`DataFrame.swaplevels` and the axis is not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just move to 1.1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@topper-123
Copy link
Contributor Author

Ping.

@jreback jreback merged commit 545381f into pandas-dev:master Jan 24, 2020
@jreback
Copy link
Contributor

jreback commented Jan 24, 2020

thanks!

@topper-123 topper-123 deleted the type_index_columns branch January 24, 2020 20:20
vipulrai91 pushed a commit to vipulrai91/pandas that referenced this pull request May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants