REF: separate indexer utilities from indexing.py #27229

jbrockmendel · 2019-07-04T14:35:15Z

A couple things going on here.

Take simple, low-dependency, not-used-in-indexing.py functions out of indexing.py and put them in indexers.py.
- A couple of runtime-defined functions in internals get moved here too.
- Avoids some runtime imports (i.e. simplify dependency structure)
- Fixes some incorrect private names
- Adds typing
Simplify some indexing.py methods
- Avoid duplicated validation calls
- Add typing
- De-duplicate _get_slice_axis method
- Move code outside of try/except blocks to clarify what the failure modes are.

Medium-term goals in indexing.py

Isolate internals access
refactor _setitem_with_indexer (over 300 lines)

…nindexing

jreback

just some quick comments
will have a full look later

+1 on refactoring things

pandas/core/indexers.py

jreback · 2019-07-04T14:42:54Z

pandas/core/indexers.py

+    return is_list_like(key) and not (isinstance(key, tuple) and type(key) is not tuple)
+
+
+def is_scalar_indexer(indexer, arr_value) -> bool:


if possible to type pls do so (may be tricky)

I would type with Any explicity if it’s truly that
so we know this is done (meaning it has been typed)

possible to use reveal_type to figure things out here

ATM I've added types for everything I had figured out with sufficient certainty. I'd rather leave it clearly-unfinished so it gets more attention than put Any if we can be more specific

can you fix this doc-sring in followup and type as much as possible here

jreback · 2019-07-04T16:49:14Z

note as a post or pre-cursor to this I would form pandas/core/indexing/ and move indexing.py (and indexers.py) there as well

jbrockmendel · 2019-07-04T17:39:43Z

note as a post or pre-cursor to this I would form pandas/core/indexing/ and move indexing.py (and indexers.py) there as well

Sure. Obvious preference for post-cursor.

…nindexing

pep8speaks · 2019-07-06T14:31:40Z

Hello @jbrockmendel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-08 13:51:16 UTC

…nindexing

jreback

just a couple of doc-strings I think (in the first section), then can merge this

jreback · 2019-07-08T01:21:08Z

pandas/core/indexing.py

-        else:
-            return self.obj._take(indexer, axis=axis)
+        indexer = self._convert_slice_indexer(slice_obj, axis)
+        assert isinstance(indexer, slice), type(indexer)


prob better off to have this assert in _slice

pandas/core/indexing.py

…nindexing

jbrockmendel · 2019-07-08T02:51:59Z

Added one more docstring, left un-docstringed is_scalar_indexer and is_empty_indexer because I'm not totally clear on what those functions are doing (possibly an extra indentation in is_scalar_indexer? it is almost identical to the last 5 lines of is_empty_indexer)

Added a note about InvalidIndexError being for geopandas.

…nindexing

jreback · 2019-07-10T16:21:42Z

pandas/core/indexers.py

+# Indexer Validation
+
+
+def check_setitem_lengths(indexer, value, values) -> None:


jreback · 2019-07-10T16:23:39Z

thanks. in followups we need to move / consolidate these things; also a typing pass would be helpful.

jbrockmendel added 10 commits July 3, 2019 18:43

implement indexers.py

bfd1b38

add types

746a3c0

cleanup

0233d42

separate out helper method; add types, remove redundnat checks

35d3055

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

c3eb149

…nindexing

blackify

b1ce472

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

7ee551c

…nindexing

types

b2a101a

move indexers

51ceb24

organize file

7efe2d1

jreback requested changes Jul 4, 2019

View reviewed changes

jreback added Clean Indexing Related to indexing on series/frames, not to indexes themselves Internals Related to non-user accessible pandas implementation labels Jul 4, 2019

jbrockmendel added 3 commits July 5, 2019 08:05

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

82561aa

…nindexing

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

a827ac2

…nindexing

isort

3322f4e

jbrockmendel added 3 commits July 6, 2019 07:34

lint+isort compat

5b6335b

blackify

74c3d36

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

f8b5264

…nindexing

jreback requested changes Jul 8, 2019

View reviewed changes

jbrockmendel added 4 commits July 7, 2019 19:42

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

42bdf94

…nindexing

remove assertion

0fafcc4

add comment for GH#27259

8b8442c

docstring

d478037

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

741c4d2

…nindexing

revert isinstance to type checks

39fa014

jbrockmendel mentioned this pull request Jul 10, 2019

REF: check can_hold_element instead of try/except #27298

Merged

jreback reviewed Jul 10, 2019

View reviewed changes

pandas/core/indexers.py

# Indexer Validation

def check_setitem_lengths(indexer, value, values) -> None:

Copy link

Contributor

jreback Jul 10, 2019

Choose a reason for hiding this comment

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

type

jreback added this to the 0.25.0 milestone Jul 10, 2019

jreback approved these changes Jul 10, 2019

View reviewed changes

jreback merged commit 298c7cc into pandas-dev:master Jul 10, 2019

jbrockmendel deleted the clnindexing branch July 10, 2019 16:27

		return is_list_like(key) and not (isinstance(key, tuple) and type(key) is not tuple)


		def is_scalar_indexer(indexer, arr_value) -> bool:

		# Indexer Validation


		def check_setitem_lengths(indexer, value, values) -> None:

Uh oh!

REF: separate indexer utilities from indexing.py #27229

REF: separate indexer utilities from indexing.py #27229

Uh oh!

Conversation

jbrockmendel commented Jul 4, 2019

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jreback Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

jbrockmendel Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

jreback Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 4, 2019

Uh oh!

jbrockmendel commented Jul 4, 2019

Uh oh!

pep8speaks commented Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2019-07-08 13:51:16 UTC

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

jreback Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbrockmendel commented Jul 8, 2019

Uh oh!

jreback Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pep8speaks commented Jul 6, 2019 •

edited

Loading