-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
BUG: assert_index_equal does not raise error for check_categorical=False when comparing 2 CategoricalIndex objects #21092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hello @alysivji! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on May 17, 2018 at 13:21 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #21092 +/- ##
==========================================
+ Coverage 91.83% 91.84% +<.01%
==========================================
Files 153 153
Lines 49495 49498 +3
==========================================
+ Hits 45454 45459 +5
+ Misses 4041 4039 -2
Continue to review full report at Codecov.
|
doc/source/whatsnew/v0.24.0.txt
Outdated
| ^^^^^^^^^^^ | ||
|
|
||
| - | ||
| - Bug in :func:`pandas.util.testing.assert_index_equal` raised ``AssertionError`` if comparing two :class:`CategoricalIndex` objects when ``check_categorical=False`` (:issue:`19776`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved to 0.23.1 since it isn't an API breaking change.
TomAugspurger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple minor things. LGTM otherwise.
pandas/tests/util/test_testing.py
Outdated
| Attribute "dtype" are different | ||
| \\[left\\]: CategoricalDtype\\(categories=\\[u?'a', u?'b'\\], ordered=False\\) | ||
| \\[right\\]: CategoricalDtype\\(categories=\\[u?'a', u?'b', u?'c'\\], ordered=False\\)""" # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a slight preference for avoiding the noqa if possible. Can you use a \ to escape the newline? Something like
expected = """Attributes are different
Attribute "dtype" are different
\\[left\\]: CategoricalDtype\\(categories=\\[u?'a', u?'b'\\], ordered=False\\)
\\[right\\]: CategoricalDtype\\(categories=\\[u?'a', u?'b', u?'c'\\], \
ordered=False\\)"""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, didn't feel right to me either. I was wondering how to split lines.
doc/source/whatsnew/v0.23.1.txt
Outdated
| Categorical | ||
| ^^^^^^^^^^^ | ||
|
|
||
| - Bug in :func:`pandas.util.testing.assert_index_equal` raised ``AssertionError`` if comparing two :class:`CategoricalIndex` objects when ``check_categorical=False`` (:issue:`19776`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which raised AssertionError incorrectly, when comparing two ....
pandas/util/testing.py
Outdated
| _check_types(left.levels[level], right.levels[level], obj=obj) | ||
|
|
||
| if check_exact: | ||
| run_exact_index_check = check_exact and check_categorical |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need a variable, just do the:
if checkexact and check_categorical:
| check_dtype : bool, default True | ||
| Check that integer dtype of the codes are the same | ||
| obj : str, default 'Categorical' | ||
| Specify object name being compared, internally used to show appropriate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you move this to make this consistent with other function orderings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you got it. There is at least one more that can be made more consistent. @jreback should I add to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
| def assert_numpy_array_equal(left, right, strict_nan=False, | ||
| check_dtype=True, err_msg=None, | ||
| obj='numpy array', check_same=None): | ||
| check_same=None, obj='numpy array'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified order of params to match other assert_* helper methods
|
thanks @alysivji |
…lse when comparing 2 CategoricalIndex objects (pandas-dev#21092) (cherry picked from commit af2b609)
…lse when comparing 2 CategoricalIndex objects (pandas-dev#21092)
git diff upstream/master -u -- "*.py" | flake8 --diffcheck_categoricalparameter inassert_series_equalassert_categorical_equalparameter order and docstring to match other functions (objat the end)Thanks to all maintainers who helped answer questions on Gitter during the informal PyCon sprint!