Fix scalar iloc rebase #17163
Fix scalar iloc rebase #17163
Conversation
gfyoung
added
Indexing
|
Wow. With #15778 , 2 circleci nodes out of 4 failed. Rebased, now only 1 fails. Trying a trivial rebase, just to be sure, sorry for the noise. |
toobaz
force-pushed
the
fix_scalar_iloc_rebase
branch
from
59475e8 to
aeb8bf7
Compare
Codecov Report
@@ Coverage Diff @@
## master #17163 +/- ##
==========================================
+ Coverage 90.99% 91% +<.01%
==========================================
Files 162 162
Lines 49508 49528 +20
==========================================
+ Hits 45052 45072 +20
Misses 4456 4456
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #17163 +/- ##
==========================================
- Coverage 91% 90.98% -0.02%
==========================================
Files 162 162
Lines 49508 49538 +30
==========================================
+ Hits 45055 45073 +18
- Misses 4453 4465 +12
Continue to review full report at Codecov.
|
|
(The failing test will be solved by #17002 - will rebase then) |
toobaz
force-pushed
the
fix_scalar_iloc_rebase
branch
from
aeb8bf7 to
5dd7bfa
Compare
|
Hello @toobaz! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on August 09, 2017 at 09:49 Hours UTC |
toobaz
force-pushed
the
fix_scalar_iloc_rebase
branch
from
5dd7bfa to
b8ee051
Compare
| @@ -1085,3 +1085,24 @@ def cast_scalar_to_array(shape, value, dtype=None): | |||
| values.fill(fill_value) | |||
|
|
|||
| return values | |||
|
|
|||
|
|
|||
| def _maybe_convert_indexer(indexer, until): | |||
There was a problem hiding this comment.
this is likely duplicating some code in pandas.core.indexing. Also doesn't belong here as its purely an indexing converter (so put it in pandas.core.indexing)
There was a problem hiding this comment.
Also doesn't belong here as its purely an indexing converter (so put it in pandas.core.indexing)
I will use it also in BlockManager: can you confirm you suggestion is still valid?
There was a problem hiding this comment.
that's fine. it just belongs in internal utlities related to indexing (and not internal casting type things). yes it is casting but is purely related to indexing (and not data types)
|
|
||
| def _maybe_convert_indexer(indexer, until): | ||
| """ | ||
| Convert slice, tuple, list or scalar "indexer" to 1-d array of indices, |
There was a problem hiding this comment.
I think it would be ok to first do a PR to add a level to our indexing structure of code, IOW
pandas.core.indexing -> pandas.core.indexing.indexing, then you add other help modules as needed
e.g. pands.core.indexing.cast for example (where things like this could live).
There was a problem hiding this comment.
Just for completeness: recall that part of this (i.e. the InfoCleaner) is temporary, until I finish the work on BlockManager (while _maybe_convert_to_indexer will stay - it is used by the BlockManager too - as well as part of the new _setter). In the long run, I expect the amount and complexity of indexing code to actually decrease. So I'm a bit against more hierarchy. Actually, if you find this PR too messy, I'd rather close it and just wait for the work on BlockManager (which however will take more time, so the bug will have to wait).
There was a problem hiding this comment.
yeah I'd rather not introduce a 'temporary' fix.
OK, then never mind. |
git diff upstream/master -u -- "*.py" | flake8 --diffThis is just a rebased version of #15778 . I want to make sense of the tests which failed, and which work fine instead on my machine.
Notice that I'm also working on a more complete approach to positional indexing, which will obsolete part of this PR, but that's going to take a while (and this PR, besides fixing the bug, is a step in that direction, since it rationalizes a bit positional indexing).