Skip to content

REF: Refactor assert_index_equal #41980

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

Merged
merged 2 commits into from
Jun 18, 2021

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented Jun 13, 2021

There are currently some issues in _check_dtypes in assert_index_equal related to #41153:

  • assert_attr_equal("dtype", left, right, obj=obj) currently isn't run if check_categorical is False (even if the indexes are not CategoricalIndexes)
  • assert_class_equal with exact='equiv' should not be restricted to accept equivalence of Int64Index and RangeIndex, but should accept equivalence of all subclasses of NumericIndex (in preparation for ENH: NumericIndex for any numpy int/uint/float dtype #41153).

So, comparing numeric indexes with assert_index_equal(idx1, idx2, exact="equiv") we should for otherwise equal content have:

  • RangeIndex and Int64Index should pass, because they're both numeric indexes and have the same dtype
  • Int64Index and Float64Index should fail, because even though they're both numeric indexes, they have different dtypes
  • Int64Index and NumericIndex[int64] should pass, because they're both numeric indexes and have the same dtype
  • RangeIndex and NumericIndex[int64] should pass, because they're both numeric indexes and have the same dtype
  • Float64Index and NumericIndex[int64] should fail, because even though they're both numeric indexes, they have different dtypes

Conversely, with exact=True class comparison should be strict, so:

  • RangeIndex and Int64Index should fail, because they're different classes, even though they have the same dtype
  • Int64Index and NumericIndex[int64] should fail, because they're different classes, even though they have the same dtype
  • etc.

This refactoring does not change behaviour in master, but will make #41153 easier.

@topper-123 topper-123 force-pushed the ref_assert_index_equal branch from 5e2197b to 2497c79 Compare June 13, 2021 09:47
@topper-123
Copy link
Contributor Author

Everything passes localy, so seems like the CI failurees are unrelated.

if check_categorical:
assert_attr_equal("dtype", left, right, obj=obj)
if is_categorical_dtype(left.dtype) and is_categorical_dtype(right.dtype):
if is_categorical_dtype(left.dtype) and is_categorical_dtype(right.dtype):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did this change any tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, right now. I think comparing dtype and inferred_type gives the same result for other dtypes that categoricals and numeric dtypes.

After #41153 we'll have int32 etc. dtypes, so then we'll have cases where it'll fail without this change.

@topper-123 topper-123 force-pushed the ref_assert_index_equal branch from 3c034a7 to c34ae66 Compare June 17, 2021 21:42
@topper-123
Copy link
Contributor Author

Rebased.

@jreback jreback added Index Related to the Index class or subclasses Testing pandas testing functions or related to the test suite labels Jun 18, 2021
@jreback jreback added this to the 1.4 milestone Jun 18, 2021
@jreback jreback merged commit 50e55e6 into pandas-dev:master Jun 18, 2021
@jreback
Copy link
Contributor

jreback commented Jun 18, 2021

thanks @topper-123

@topper-123 topper-123 deleted the ref_assert_index_equal branch June 18, 2021 05:22
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Index Related to the Index class or subclasses Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants