Avoid accessing private loc methods #27383
Conversation
|
@WillAyd can you help me make heads or tails out of the mypy complaint? |
|
Yea I imagine it's because of part of Line 1745: [key] + [slice(None)]So the left operand is inferred to Could either:
I think option 1 is better since this code is shadowing |
The part that I don't get is why is it only complaining now? The types being added should be a strict improvement typing-wise, right? |
|
Haven’t reviewed in detail but if first annotations in module Mypy will actually start checking. Before may have just skipped
…Sent from my iPhone
On Jul 14, 2019, at 8:50 AM, jbrockmendel ***@***.***> wrote:
Yea I imagine it's because of part of Line 1745:
The part that I don't get is why is it only complaining now? The types being added should be a strict improvement typing-wise, right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
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-23 23:08:04 UTC |
pandas/core/indexing.py
Outdated
| @@ -222,7 +223,7 @@ def _validate_key(self, key, axis: int): | |||
| """ | |||
| raise AbstractMethodError(self) | |||
|
|
|||
| def _has_valid_tuple(self, key): | |||
| def _has_valid_tuple(self, key: tuple): | |||
There was a problem hiding this comment.
Can you use the generic Tuple from the typing module? Providing the type(s) contained within would be even better (maybe Tuple[Hashable]?)
pandas/core/indexing.py
Outdated
| """Translate any partial string timestamp matches in key, returning the | ||
| new key (GH 10331)""" | ||
| if isinstance(labels, MultiIndex): | ||
| if isinstance(key, str) and labels.levels[0].is_all_dates: | ||
| # Convert key '2016-01-01' to | ||
| # ('2016-01-01'[, slice(None, None, None)]+) | ||
| key = tuple([key] + [slice(None)] * (len(labels.levels) - 1)) | ||
| key = tuple( | ||
| [key] + [slice(None)] * (len(labels.levels) - 1) |
There was a problem hiding this comment.
The more I think about this a better idiom would be something like:
new_key = None # type: List[str, slice]
if isinstance(labels, MultiIndex):
...
new_key = tuple([key] + [slice(None)] * len(labels.levels) - 1)
...Should appease mypy and prevent the shadowing of the key variable
| @@ -3484,7 +3484,9 @@ def _setitem_array(self, key, value): | |||
| for k1, k2 in zip(key, value.columns): | |||
| self[k1] = value[k2] | |||
| else: | |||
| indexer = self.loc._convert_to_indexer(key, axis=1) | |||
| indexer = self.loc._get_listlike_indexer( | |||
There was a problem hiding this comment.
self.columns.get_indexer(key)?
There was a problem hiding this comment.
Looks like this returns something different, will have to keep digging
jreback
added
Clean
Indexing
Codecov Report
@@ Coverage Diff @@
## master #27383 +/- ##
==========================================
- Coverage 92.91% 92.89% -0.02%
==========================================
Files 184 184
Lines 50385 50387 +2
==========================================
- Hits 46813 46809 -4
- Misses 3572 3578 +6
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #27383 +/- ##
==========================================
- Coverage 92.91% 92.89% -0.02%
==========================================
Files 184 184
Lines 50385 50387 +2
==========================================
- Hits 46813 46809 -4
- Misses 3572 3578 +6
Continue to review full report at Codecov.
|
|
gentle ping; there are (requested) follow-ups |
|
thanks |
Works toward fixing #27344
Adds types where unambiguous