Add typing annotation to assert_index_equal#26535
Merged
jreback merged 11 commits intopandas-dev:masterfrom Jun 8, 2019
Merged
Add typing annotation to assert_index_equal#26535jreback merged 11 commits intopandas-dev:masterfrom
jreback merged 11 commits intopandas-dev:masterfrom
Conversation
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
| 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): |
Contributor
There was a problem hiding this comment.
this is needed to avoid type issues?
Contributor
Author
There was a problem hiding this comment.
Yes. Otherwise, mypy showed
pandas/util/testing.py:608: error: "Index" has no attribute "levels"; maybe "nlevels"?
Member
There was a problem hiding this comment.
Shouldn't this just replace the existing condition then? Seems duplicative to have both checks
Member
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
Member
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
We explicitly exclude the tests directory from mypy right now (see mypy.ini). Could you check any impacts of removing that from the blacklist as part of this?
Contributor
Author
|
Lines 5 to 6 in 19f693f
|
Contributor
Author
|
After removing |
WillAyd
requested changes
May 28, 2019
| check_names: bool = True, | ||
| check_less_precise: Union[bool, int] = False, | ||
| check_exact: bool = True, | ||
| check_categorical=True, |
Member
There was a problem hiding this comment.
Can you annotate this parameter as well?
| 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): |
Member
There was a problem hiding this comment.
Shouldn't this just replace the existing condition then? Seems duplicative to have both checks
Contributor
Author
|
@WillAyd Would you tell me more? I don't understand what you mean. |
topper-123
requested changes
May 29, 2019
| check_less_precise: Union[bool, int] = False, | ||
| check_exact: bool = True, | ||
| check_categorical: bool = True, | ||
| obj: str = 'Index'): |
Contributor
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, |
Contributor
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.
Contributor
There was a problem hiding this comment.
this is ok as all of these are Index subclasses
Contributor
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.
Member
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
Member
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
Couple comments and one update request
| check_less_precise=False, check_exact=True, | ||
| check_categorical=True, obj='Index'): | ||
| def assert_index_equal(left: Index, | ||
| right: Index, |
Member
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: |
Member
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
| # 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: |
Member
There was a problem hiding this comment.
I still think this line should be removed
Member
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
Contributor
There was a problem hiding this comment.
yeah maybe the cast inside the condition is better?
Contributor
Author
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.
Contributor
There was a problem hiding this comment.
Member
|
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
| check_less_precise=check_less_precise, | ||
| check_exact=check_exact, obj=lobj) | ||
| # get_level_values may change dtype | ||
| left = cast(MultiIndex, left) |
Member
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
| check_less_precise: Union[bool, int] = False, | ||
| check_exact: bool = True, | ||
| check_categorical: bool = True, | ||
| obj: str = 'Index') -> Optional[None]: |
Member
There was a problem hiding this comment.
Can just do None for the return (Optional[None] I don't think makes sense)
Contributor
Author
|
Something wrong with CI. Travis test can't be finished. |
Contributor
Author
|
@WillAyd All tests passed. |
WillAyd
approved these changes
Jun 8, 2019
jreback
approved these changes
Jun 8, 2019
Contributor
|
thanks @makbigc very nice! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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