Add typing annotation to assert_index_equal #26535
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #26535 +/- ##
==========================================
- Coverage 91.76% 91.76% -0.01%
==========================================
Files 174 174
Lines 50629 50631 +2
==========================================
- Hits 46462 46461 -1
- Misses 4167 4170 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26535 +/- ##
==========================================
- Coverage 91.78% 91.77% -0.01%
==========================================
Files 174 174
Lines 50703 50706 +3
==========================================
- Hits 46538 46537 -1
- Misses 4165 4169 +4
Continue to review full report at Codecov.
|
jreback
requested changes
May 27, 2019
pandas/util/testing.py
Outdated
| check_exact=check_exact, obj=lobj) | ||
| # get_level_values may change dtype | ||
| _check_types(left.levels[level], right.levels[level], obj=obj) | ||
| if isinstance(left, MultiIndex) and isinstance(right, MultiIndex): |
There was a problem hiding this comment.
this is needed to avoid type issues?
There was a problem hiding this comment.
Yes. Otherwise, mypy showed
pandas/util/testing.py:608: error: "Index" has no attribute "levels"; maybe "nlevels"?
There was a problem hiding this comment.
Shouldn't this just replace the existing condition then? Seems duplicative to have both checks
There was a problem hiding this comment.
My point is that if isinstance(left, MultiIndex) and if left.nlevels > 1 are equivalent so no need to have both (just the former).
Also does this have any impact on assertions given the check previously only looked at the left index and not both?
WillAyd
requested changes
May 27, 2019
|
Lines 5 to 6 in 19f693f
|
|
After removing |
WillAyd
requested changes
May 28, 2019
pandas/util/testing.py
Outdated
| check_names: bool = True, | ||
| check_less_precise: Union[bool, int] = False, | ||
| check_exact: bool = True, | ||
| check_categorical=True, |
There was a problem hiding this comment.
Can you annotate this parameter as well?
pandas/util/testing.py
Outdated
| check_exact=check_exact, obj=lobj) | ||
| # get_level_values may change dtype | ||
| _check_types(left.levels[level], right.levels[level], obj=obj) | ||
| if isinstance(left, MultiIndex) and isinstance(right, MultiIndex): |
There was a problem hiding this comment.
Shouldn't this just replace the existing condition then? Seems duplicative to have both checks
|
@WillAyd Would you tell me more? I don't understand what you mean. |
topper-123
requested changes
May 29, 2019
pandas/util/testing.py
Outdated
| check_less_precise: Union[bool, int] = False, | ||
| check_exact: bool = True, | ||
| check_categorical: bool = True, | ||
| obj: str = 'Index'): |
There was a problem hiding this comment.
The return type should be None.
| check_less_precise=False, check_exact=True, | ||
| check_categorical=True, obj='Index'): | ||
| def assert_index_equal(left: Index, | ||
| right: Index, |
There was a problem hiding this comment.
Instead of a raw Index, is it possible to add a typing.TypeVar to _typing.py:
AnyIndex = TypeVar('AnyIndex ', pd.Index, pd.RangeIndex, pd.Int64Index, pd.MultiIndex, + other index types...)and then use that here (I'm not super aqianted with typing, so could be wrong)?
That would avoid the need to adapt the code below.
There was a problem hiding this comment.
this is ok as all of these are Index subclasses
There was a problem hiding this comment.
The point was that AnyIndex would capture .levels and other attributes that are not set on the base Index.
There was a problem hiding this comment.
I think this is OK too. MyPy would still complain about those attributes not existing on Index
WillAyd
requested changes
May 30, 2019
| check_less_precise=False, check_exact=True, | ||
| check_categorical=True, obj='Index'): | ||
| def assert_index_equal(left: Index, | ||
| right: Index, |
There was a problem hiding this comment.
I think this is OK too. MyPy would still complain about those attributes not existing on Index
| check_less_precise: Union[bool, int] = False, | ||
| check_exact: bool = True, | ||
| check_categorical: bool = True, | ||
| obj: str = 'Index') -> None: |
There was a problem hiding this comment.
Just a general comment but NoReturn may be applicable here at some point when we drop 3.5, though not sure if Optional[NoReturn] makes sense
pandas/util/testing.py
Outdated
| # get_level_values may change dtype | ||
| _check_types(left.levels[level], right.levels[level], obj=obj) | ||
| if isinstance(left, MultiIndex) and isinstance(right, MultiIndex): | ||
| if left.nlevels > 1: |
There was a problem hiding this comment.
I still think this line should be removed
There was a problem hiding this comment.
Or alternately we could keep original code in tact and simply cast inside of this condition; that would prevent any actual code changes but give mypy the inference it needs that we are only working with MI in this branch.
I'd actually prefer that but @jreback may have differing thoughts
There was a problem hiding this comment.
yeah maybe the cast inside the condition is better?
There was a problem hiding this comment.
Pardon me. I am not familiar with casting in Python. By casting, do you mean left = MuliIndex(left)? This is a construction of a new MuliIndex which result in type error in our case TypeError: Must pass both levels and labels.
Or one more type annotation?
left # type: MultiIndex
right # type: MultiIndex
_check_types(left.levels[level], right.levels[level], obj=obj)
mypy still regards left and right as Index.
There was a problem hiding this comment.
|
There is a cast function in the standard typing module you can use
…Sent from my iPhone
On Jun 2, 2019, at 10:42 AM, Mak Sze Chun ***@***.***> wrote:
@makbigc commented on this pull request.
In pandas/util/testing.py:
> @@ -587,19 +593,20 @@ def _get_ilevel_values(index, level):
raise_assert_detail(obj, msg1, msg2, msg3)
# MultiIndex special comparison for little-friendly error messages
- if left.nlevels > 1:
- for level in range(left.nlevels):
- # cannot use get_level_values here because it can change dtype
- llevel = _get_ilevel_values(left, level)
- rlevel = _get_ilevel_values(right, level)
-
- lobj = 'MultiIndex level [{level}]'.format(level=level)
- assert_index_equal(llevel, rlevel,
- exact=exact, check_names=check_names,
- check_less_precise=check_less_precise,
- check_exact=check_exact, obj=lobj)
- # get_level_values may change dtype
- _check_types(left.levels[level], right.levels[level], obj=obj)
+ if isinstance(left, MultiIndex) and isinstance(right, MultiIndex):
+ if left.nlevels > 1:
Pardon me. I am not familiar with casting in Python. By casting, do you mean left = MuliIndex(left)? This is a construction of a new MuliIndex which result in type error in our case TypeError: Must pass both levels and labels.
Or one more type annotation?
left # type: MultiIndex
right # type: MultiIndex
_check_types(left.levels[level], right.levels[level], obj=obj)
mypy still regards left and right as Index.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
WillAyd
requested changes
Jun 3, 2019
pandas/util/testing.py
Outdated
| @@ -599,6 +605,8 @@ def _get_ilevel_values(index, level): | |||
| check_less_precise=check_less_precise, | |||
| check_exact=check_exact, obj=lobj) | |||
| # get_level_values may change dtype | |||
| left = cast(MultiIndex, left) | |||
There was a problem hiding this comment.
Can you move these to line 597? That is specifically where the inference of a new type should occur would logically fit better there
WillAyd
requested changes
Jun 4, 2019
pandas/util/testing.py
Outdated
| check_less_precise: Union[bool, int] = False, | ||
| check_exact: bool = True, | ||
| check_categorical: bool = True, | ||
| obj: str = 'Index') -> Optional[None]: |
There was a problem hiding this comment.
Can just do None for the return (Optional[None] I don't think makes sense)
|
Something wrong with CI. Travis test can't be finished. |
|
@WillAyd All tests passed. |
WillAyd
approved these changes
Jun 8, 2019
jreback
approved these changes
Jun 8, 2019
|
thanks @makbigc very nice! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git diff upstream/master -u -- "*.py" | flake8 --diff