REGR: check_flags not respected in assert_frame_equal #45565
Conversation
phofl
commented
Jan 23, 2022
phofl
commented
- closes BUG: Parameter check_flags is ignored in assert_frame_equal #45554
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
phofl
added
Testing
| left.flags.allows_duplicate_labels = False | ||
|
|
||
| right = DataFrame([[1, 2], [3, 4]]) | ||
| right.flags.allows_duplicate_labels = True |
There was a problem hiding this comment.
do we also need to test for DataFrames with the same flags with check_flags=True/False. I find it odd (without investigation) that the reported issue was not picked up before. The flag is on the DataFrame so why not check that instead of on the Series?
doc/source/whatsnew/v1.4.1.rst
Outdated
| @@ -15,7 +15,7 @@ including other versions of pandas. | |||
| Fixed regressions | |||
| ~~~~~~~~~~~~~~~~~ | |||
| - Regression in :meth:`Series.mask` with ``inplace=True`` and ``PeriodDtype`` and an incompatible ``other`` coercing to a common dtype instead of raising (:issue:`45546`) | |||
| - | |||
| - Regression in :func:`.assert_frame_equal` not respecting ``check_flags=False``(:issue:`45554`) | |||
There was a problem hiding this comment.
again. I don't understand fully since have not investigated but this issue is not actually a regression if the flag was not propagated before? As with check_flags=True I assume it did not raise if the flags were different.
pandas/_testing/asserters.py
Outdated
| @@ -1344,6 +1344,7 @@ def assert_frame_equal( | |||
| rtol=rtol, | |||
| atol=atol, | |||
| check_index=False, | |||
| check_flags=check_flags, | |||
There was a problem hiding this comment.
right. I think I am starting to get my head round this. This should be check_flags=False to override the default checking in assert_series_equal and be consistent with check_index=False in the line above.
the flags check is already performed on the DataFrame.
There was a problem hiding this comment.
Yep you are correct. Passing in False is the way to go here
|
Also, we should maybe consider #45092 a bug-fix for the flags propagation to the Series object and add some tests for that? I guess this is tested somewhere if the propagation of the flags to the Series when indexing a DataFrame is the right thing to do. |
There was a problem hiding this comment.
blocking for now. The regression is from #45092 so that's the starting point. OK that the changes here may seem sensible but I think needs more discussion and merging this will close off the issue.
As #45092 appears to have made an unintentional change we should consider the implications. (is is the right thing to do, is it fully tested, should it have been communicated, do the docs need updating, is it a breaking change)
we may want to revert #45092 for now and re-introduce in 1.5
|
Hm in general I am fine with #45092, since the flags are now propagated because of the added finalize call. Which is what we should expect I think. I don't think that we have many tests where we check for the flags (might be wrong here), because this is related to the finalize issue, where we check for metadata. If the metadata are propagated, the flag is propagated too. |
agree, yeah we should actcually turn this to true as a default if possible. |
| @@ -15,7 +15,7 @@ including other versions of pandas. | |||
| Fixed regressions | |||
| ~~~~~~~~~~~~~~~~~ | |||
| - Regression in :meth:`Series.mask` with ``inplace=True`` and ``PeriodDtype`` and an incompatible ``other`` coercing to a common dtype instead of raising (:issue:`45546`) | |||
| - | |||
| - Regression in :func:`.assert_frame_equal` not respecting ``check_flags=False`` (:issue:`45554`) | |||
There was a problem hiding this comment.
Borrowed it from v1.3.0, worked there
|
@meeseeksdev backport 1.4.x |
|
Something went wrong ... Please have a look at my logs. |
…qual (#45671) Co-authored-by: Patrick Hoefler <[email protected]>
