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

REF: use sanitize_array in Index.__new__ #49718

Merged
merged 9 commits into from
Nov 18, 2022

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Nov 16, 2022

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

i.e. share code paths with Series constructor.

This isn't Technically Correct until #49348

@mroeschke mroeschke added Refactor Internal refactoring of code Index Related to the Index class or subclasses labels Nov 16, 2022
@jbrockmendel
Copy link
Member Author

pandas/core/indexes/base.py:216:8: R1720: Unnecessary "elif" after "raise", remove the leading "el" from "elif" (no-else-raise)

@MarcoGorelli are you the right person to complain to about this? i hereby complain.

@MarcoGorelli MarcoGorelli self-requested a review November 16, 2022 18:18
@MarcoGorelli
Copy link
Member

yup, removing this one now

# cast to list here
data = list(data)
if len(data) == 0:
# unlike Series, we default to object dtype:
Copy link
Member

Choose a reason for hiding this comment

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

Would we want to align this in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

i have no strong opinion on this

@@ -208,6 +206,22 @@
_dtype_obj = np.dtype("object")


def _wrapped_sanitize(cls, data, dtype: DtypeObj | None, copy: bool):
Copy link
Member

Choose a reason for hiding this comment

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

So ideally we wouldn't need this in the future once there's more alignment between Series and Index?

Copy link
Member Author

Choose a reason for hiding this comment

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

the strict_ints keyword we could get rid of after #49609. the rest of this wrapping is mostly about giving Index-specific exception messages, so is pretty benign

(actually, at the time i refactored this function out i was calling it 3ish times within Index.__new__. now that its only called once, we could inline it)

Copy link
Member

Choose a reason for hiding this comment

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

Inlining can happen in a follow up

@mroeschke mroeschke added this to the 2.0 milestone Nov 18, 2022
@mroeschke mroeschke merged commit 8020bf1 into pandas-dev:main Nov 18, 2022
@mroeschke
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the ref-index_new branch November 18, 2022 02:24
MarcoGorelli pushed a commit to MarcoGorelli/pandas that referenced this pull request Nov 18, 2022
* REF: Index.__new__ use sanitize_array

* REF: _wrapped_sanitize

* re-use wrapped_sanitize

* cln

* REF: share

* avoid extra copy

* troubleshoot CI

* pylint fixup
mliu08 pushed a commit to mliu08/pandas that referenced this pull request Nov 27, 2022
* REF: Index.__new__ use sanitize_array

* REF: _wrapped_sanitize

* re-use wrapped_sanitize

* cln

* REF: share

* avoid extra copy

* troubleshoot CI

* pylint fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Index Related to the Index class or subclasses Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants