Skip to content

Fix obj arguments in assertions #59460

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.

Sign up for GitHub

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 4 commits into from
Sep 3, 2024
Merged

Fix obj arguments in assertions #59460

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6dfe24
Fix obj arguments in assertions
serl Aug 9, 2024
4c0c893
Add test for assert_interval_array_equal
serl Aug 23, 2024
09a1ae5
Still say MultiIndex in some cases
serl Aug 27, 2024
ad99297
Be a little braver and change the original obj definition
serl Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pandas/_testing/asserters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ def _check_types(left, right, obj: str = "Index") -> None:
right = cast(MultiIndex, right)

for level in range(left.nlevels):
lobj = f"MultiIndex level [{level}]"
lobj = f"{obj} level [{level}]"
try:
# try comparison on levels/codes to avoid densifying MultiIndex
assert_index_equal(
Expand Down Expand Up @@ -314,7 +314,7 @@ def _check_types(left, right, obj: str = "Index") -> None:
obj=lobj,
)
# get_level_values may change dtype
_check_types(left.levels[level], right.levels[level], obj=obj)
_check_types(left.levels[level], right.levels[level], obj=lobj)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't manage to write a test for this (type seems to be already checked just above), which, given its comment, smells very corner-case-y. Probably it's better to stop the head-banging, unless you have ideas on a test case, of course


# skip exact index checking when `check_categorical` is False
elif check_exact and check_categorical:
Expand Down Expand Up @@ -527,7 +527,7 @@ def assert_interval_array_equal(
kwargs["check_freq"] = False

assert_equal(left._left, right._left, obj=f"{obj}.left", **kwargs)
assert_equal(left._right, right._right, obj=f"{obj}.left", **kwargs)
assert_equal(left._right, right._right, obj=f"{obj}.right", **kwargs)

assert_attr_equal("closed", left, right, obj=obj)

Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/util/test_assert_frame_equal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def test_frame_equal_shape_mismatch(df1, df2, frame_or_series):
DataFrame.from_records(
{"a": [1.0, 2.0], "b": [2.1, 1.5], "c": ["l1", "l2"]}, index=["a", "b"]
),
"MultiIndex level \\[0\\] are different",
"DataFrame\\.index level \\[0\\] are different",
),
],
)
Expand Down
12 changes: 5 additions & 7 deletions pandas/tests/util/test_assert_index_equal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ def test_index_equal_levels_mismatch():


def test_index_equal_values_mismatch(check_exact):
msg = """MultiIndex level \\[1\\] are different
msg = """Index level \\[1\\] are different

MultiIndex level \\[1\\] values are different \\(25\\.0 %\\)
Index level \\[1\\] values are different \\(25\\.0 %\\)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I really like this change. I think MultiIndex level is clearer here

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 see. Just to be on the same page, have you got the opportunity to skim #59440?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so I tried to make a compromise (09a1ae5), which in short is:

root_obj = "MultiIndex" if obj == "Index" else obj
...
lobj = f"{root_obj} level [{level}]"

I find that a little brittle, as it is based on the fact that about a hundred lines before there is obj: str = "Index".

...So I tried harder (ad99297), which correspond to the current state of the PR. That is having the obj in the assert_index_equal function definition being None by default and branching immediately:

    if obj is None:
        obj = "MultiIndex" if isinstance(left, MultiIndex) else "Index"

Thoughts? Better ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @mroeschke, have you got the chance to check this? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I really like this change. I think MultiIndex level is clearer here

No disagreement that it is slightly clearer, but it bucks the otherwise consistent behavior of assert_.*_equal utilizing obj when passed.

\\[left\\]: Index\\(\\[2, 2, 3, 4\\], dtype='int64'\\)
\\[right\\]: Index\\(\\[1, 2, 3, 4\\], dtype='int64'\\)"""

Expand Down Expand Up @@ -172,9 +172,9 @@ def test_index_equal_level_values_mismatch(check_exact, rtol):
idx2 = MultiIndex.from_tuples([("A", 1), ("A", 2), ("B", 3), ("B", 4)])
kwargs = {"check_exact": check_exact, "rtol": rtol}

msg = """MultiIndex level \\[1\\] are different
msg = """Index level \\[1\\] are different

MultiIndex level \\[1\\] values are different \\(25\\.0 %\\)
Index level \\[1\\] values are different \\(25\\.0 %\\)
\\[left\\]: Index\\(\\[2, 2, 3, 4\\], dtype='int64'\\)
\\[right\\]: Index\\(\\[1, 2, 3, 4\\], dtype='int64'\\)"""

Expand Down Expand Up @@ -311,9 +311,7 @@ def test_assert_multi_index_dtype_check_categorical(check_categorical):
idx1 = MultiIndex.from_arrays([Categorical(np.array([1, 2], dtype=np.uint64))])
idx2 = MultiIndex.from_arrays([Categorical(np.array([1, 2], dtype=np.int64))])
if check_categorical:
with pytest.raises(
AssertionError, match=r"^MultiIndex level \[0\] are different"
):
with pytest.raises(AssertionError, match=r"^Index level \[0\] are different"):
tm.assert_index_equal(idx1, idx2, check_categorical=check_categorical)
else:
tm.assert_index_equal(idx1, idx2, check_categorical=check_categorical)
21 changes: 20 additions & 1 deletion pandas/tests/util/test_assert_interval_array_equal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import pytest

from pandas import interval_range
from pandas import (
Interval,
interval_range,
)
import pandas._testing as tm
from pandas.arrays import IntervalArray


@pytest.mark.parametrize(
Expand Down Expand Up @@ -79,3 +83,18 @@ def test_interval_array_equal_start_mismatch():

with pytest.raises(AssertionError, match=msg):
tm.assert_interval_array_equal(arr1, arr2)


def test_interval_array_equal_end_mismatch_only():
arr1 = IntervalArray([Interval(0, 1), Interval(0, 5)])
arr2 = IntervalArray([Interval(0, 1), Interval(0, 6)])

msg = """\
IntervalArray.right are different

IntervalArray.right values are different \\(50.0 %\\)
\\[left\\]: \\[1, 5\\]
\\[right\\]: \\[1, 6\\]"""

with pytest.raises(AssertionError, match=msg):
tm.assert_interval_array_equal(arr1, arr2)
2 changes: 1 addition & 1 deletion pandas/tests/util/test_assert_series_equal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def test_less_precise(data1, data2, any_float_dtype, decimals):
DataFrame.from_records(
{"a": [1.0, 2.0], "b": [2.1, 1.5], "c": ["l1", "l2"]}, index=["a", "b"]
).c,
"MultiIndex level \\[0\\] are different",
"Series\\.index level \\[0\\] are different",
),
],
)
Expand Down