-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
TST: GH30999 Add match=msg to all pytest.raises in tests/indexes #38697
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
Changes from 4 commits
4cc2668
02cbc37
d968db1
4fd5b91
5937097
32eca9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,14 +221,12 @@ def test_difference_sort_incomparable(): | |
tm.assert_index_equal(result, idx) | ||
|
||
|
||
@pytest.mark.xfail(reason="Not implemented.") | ||
def test_difference_sort_incomparable_true(): | ||
# TODO decide on True behaviour | ||
# # sort=True, raises | ||
idx = pd.MultiIndex.from_product([[1, pd.Timestamp("2000"), 2], ["a", "b"]]) | ||
other = pd.MultiIndex.from_product([[3, pd.Timestamp("2000"), 4], ["c", "d"]]) | ||
|
||
with pytest.raises(TypeError): | ||
msg = "The 'sort' keyword only takes the values of None or False; True was passed." | ||
with pytest.raises(ValueError, match=msg): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test was marked as failing. But when I switched it to checking that a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fine, a lot of things have been cleaned up recently I guess this was missed. cc @jbrockmendel |
||
idx.difference(other, sort=True) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,8 +49,7 @@ def test_numpy_ufuncs_basic(index, func): | |
# https://numpy.org/doc/stable/reference/ufuncs.html | ||
|
||
if isinstance(index, DatetimeIndexOpsMixin): | ||
# raise TypeError or ValueError (PeriodIndex) | ||
with pytest.raises(Exception): | ||
with tm.external_error_raised((TypeError, AttributeError)): | ||
with np.errstate(all="ignore"): | ||
func(index) | ||
elif isinstance(index, (Float64Index, Int64Index, UInt64Index)): | ||
|
@@ -66,7 +65,7 @@ def test_numpy_ufuncs_basic(index, func): | |
if len(index) == 0: | ||
pass | ||
else: | ||
with pytest.raises(Exception): | ||
with tm.external_error_raised((TypeError, AttributeError)): | ||
with np.errstate(all="ignore"): | ||
func(index) | ||
|
||
|
@@ -77,6 +76,11 @@ def test_numpy_ufuncs_basic(index, func): | |
def test_numpy_ufuncs_other(index, func, request): | ||
# test ufuncs of numpy, see: | ||
# https://numpy.org/doc/stable/reference/ufuncs.html | ||
msg = ( | ||
f"ufunc '{func.__name__}' not supported for the input types, and the " | ||
"inputs could not be safely coerced to any supported types according " | ||
"to the casting rule ''safe''" | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one's also an external error from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
if isinstance(index, (DatetimeIndex, TimedeltaIndex)): | ||
if isinstance(index, DatetimeIndex) and index.tz is not None: | ||
|
@@ -96,13 +100,11 @@ def test_numpy_ufuncs_other(index, func, request): | |
result = func(index) | ||
assert isinstance(result, np.ndarray) | ||
else: | ||
# raise TypeError or ValueError (PeriodIndex) | ||
with pytest.raises(Exception): | ||
with pytest.raises(TypeError, match=msg): | ||
func(index) | ||
|
||
elif isinstance(index, PeriodIndex): | ||
# raise TypeError or ValueError (PeriodIndex) | ||
with pytest.raises(Exception): | ||
with pytest.raises(TypeError, match=msg): | ||
func(index) | ||
|
||
elif isinstance(index, (Float64Index, Int64Index, UInt64Index)): | ||
|
@@ -114,5 +116,5 @@ def test_numpy_ufuncs_other(index, func, request): | |
if len(index) == 0: | ||
pass | ||
else: | ||
with pytest.raises(Exception): | ||
with pytest.raises(TypeError, match=msg): | ||
func(index) |
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 is a test that was marked as failing, with the reason being closed issue #15832 . I put a bit of discussion about this over in the issue.
I removed the
xfail
and I also removed the last twopytest.raises
. The result of the coercion is:IntervalIndex([(0, 0], (0, 0], (0, 0], (0, 1], (1, 1] ... (8, 9], (9, 9], (9, 9], (9, 9], (9, 10]], closed='right', dtype='interval[int64]')
which doesn't look particularly useful but isn't exactly wrong either. If I understand the discussion at #15832 correctly this is intended behaviour for it not to raise the error.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.
Looks like the test for
is lost here?
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.
Yes, I removed it deliberately, as well as the equivalent one for
uint64
. I tried to explain that in my comment above but I guess I wasn't clear.This test was marked as failing before. Now it is not. The test, as written before, was saying that
should raise a
ValueError
. It does not raise any error. The question here is: Should casting an interval range with non-integer float boundaries raise an error? This test said it should, but it was marked as failing, because right now it does not raise an error. If I understood the discussion at #15832 correctly, and it is possible that I did not, than I think that it should not raise an error, so this test is not required.The question is, as a user of pandas, if you had an interval range such as
[(0.0, 0.25], (0.25, 0.5], (0.5, 0.75], (0.75, 1.0], (1.0, 1.25], (1.25, 1.5], (1.5, 1.75], (1.75, 2.0], (2.0, 2.25], (2.25, 2.5]]
, and you cast it to int, what would you want or expect? The old (failing) version of the test said you expect a ValueError. I am saying you expect[(0, 0], (0, 0], (0, 0], (0, 1], (1, 1], (1, 1], (1, 1], (1, 2], (2, 2], (2, 2]]
which, as I said, looks not that useful as an IntervalIndex, but is not wrong either.Would love to hear your thoughts.
I could also add another test for the expected behaviour of casting from float to int intervals if that's what we want.
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.
Thanks for explaining, sorry should've read your comment on that linked issue. Your logic makes sense to me, but I'd defer to someone with more experience on how we handle
IntervalIndex
.If being removed from this test, we definitely want to make sure this specific casting behavior is being tested somewhere. To keep this pr simple and easier to merge, I'd suggest reverting modification of this test, then making a followup pr to clean up this test and add tests for any untested casting behavior.
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.
Ok, but I am not quite sure how to handle it. I want to leave the code in a good state even if I plan to do another PR soon, because it's possible that one will take a while or not be accepted ever or whatever. The way I see it, these are our options:
ValueError
, if in the future pandas were to raise one. This is the strategy I took in previous pull requests. (see TST: GH30999 Add match=msg to all "with pytest.raises" in pandas/tests/io/pytables/test_store.py #38609 for example, where I added a new completely invented error message totest_multiple_open_close
). This is also what I was trying to do that led me to read the code and the past issues and decide that I didn't think it deserved an error message at all and therefore that the test didn't make sense and I would remove it.Anyway, after thinking it through and writing it out, I think I will try for strategy 3, and you can decide if you like whatever message I come up with, but let me know if there's a different strategy that you prefer.
Apologies for the wall of text; I am new to contributing to pandas and obviously spent a lot of time thinking about this.
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.
ok done
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.
xref #38718
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.
New PR #38719
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.
If that's been done in past PR's, probably fine here too. Appreciate the thoughtfulness on your approach here!
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.
yeah generally addressing on a new PR is the correct way, will look at that soon.