Fix: EA attribute specifying whether copy=False is ignored#63130
Fix: EA attribute specifying whether copy=False is ignored#63130Aniketsy wants to merge 21 commits into
Conversation
|
@jbrockmendel Could you please take a look in this, If i'm not in wrong direction. |
|
@jbrockmendel Please review these changes, whenever you get a chance. |
| data.fillna(data_missing[1], copy=False) | ||
| tm.assert_extension_array_equal(data, data_missing) | ||
| if self._supports_fillna_copy_false: | ||
| # but with copy=False, this raises for EAs that respect the copy keyword |
There was a problem hiding this comment.
i think this comment should be for EAs that dont respect the copy keyword?
There was a problem hiding this comment.
I've added a comment in the else block for the EAs that don't respect the copy keyword.
There was a problem hiding this comment.
ok but isnt the comment here still wrong?
There was a problem hiding this comment.
i thought about removing that comment earlier but wasn’t fully sure since it was written specifically for the if case. You're right, so I’ll remove it.
| result = data.fillna(data_missing[1]) | ||
| assert result[0] == data_missing[1] | ||
| res_copy = data.fillna(fill_value, copy=True) | ||
| tm.assert_extension_array_equal(res_copy, expected) |
There was a problem hiding this comment.
can you keep using data_missing instead of defining a new expected. i.e. minimize the diff
|
@jbrockmendel I've updated the code as per your suggestion. Please let me know if this also needs a whatsnew entry. |
|
@jbrockmendel Is this PR ready now? The two ci-failures seem to be expected, since they’re caused by these. |
|
Hi @jbrockmendel , just a gentle reminder on this when you get a chance. Thanks! |
| res_copy = data.fillna(fill_value, copy=True) | ||
| tm.assert_extension_array_equal( | ||
| res_copy, data_missing.fillna(fill_value, copy=True) | ||
| ) |
There was a problem hiding this comment.
why is this assertion added? seems meaningless
There was a problem hiding this comment.
yes, this can be removed. thanks!
| @@ -6,6 +6,8 @@ | |||
|
|
|||
|
|
|||
| class BaseMissingTests: | |||
| _supports_fillna_copy_false = True | |||
There was a problem hiding this comment.
should the name say "respect" instead of "support"?
There was a problem hiding this comment.
in this not fully sure, but I’ve updated it as per your suggestion.
|
I took a help of llm for fixing ci-fails, and updated with some changes. |
| filled : IntervalArray with NA/NaN filled | ||
| """ | ||
| if copy is False: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
IntervalArray.fillna always copies regardless of the copy keyword, so i think NotImplementedError is not needed removing this will pass without error, and behavior stays, same. I would like to know your thoughts on this.
| def fillna(self, value=None, limit=None, copy: bool = True): | ||
| # We intentionally ignore copy and always perform a copy to ensure | ||
| # the original array is not modified. | ||
| return super().fillna(value=value, limit=limit, copy=True) |
There was a problem hiding this comment.
or raise/warn if the user passes copy=False
There was a problem hiding this comment.
pass copy=copy?
i think we can go with this . I'll update
|
FAILED pandas/tests/extension/decimal/test_decimal.py::TestDecimalArray::test_fillna_frame - AssertionError: Did not see expected warning of class 'Pandas4Warning' |
@jbrockmendel as we have passed copy directly, i think we can remove these test, please let me know your thoughts on this. |
|
|
||
| def test_fillna_readonly(self, data_missing): | ||
| pa_dtype = data_missing.dtype.pyarrow_dtype | ||
| if pa.types.is_duration(pa_dtype): |
There was a problem hiding this comment.
its ugly but you could patch _respects_fillna_copy_false inside a context
| data = data_missing.copy() | ||
| data._readonly = True | ||
|
|
||
| # by default fillna(copy=True), then this works fine | ||
| result = data.fillna(data_missing[1]) | ||
| assert result[0] == data_missing[1] |
There was a problem hiding this comment.
can you restore this line somewhere (to check that the NAs actually got filled)
There was a problem hiding this comment.
tm.assert_extension_array_equal(res_no_copy, res_copy), i think this already check, i also checked by restoring and got this failure on testing
def test_fillna_readonly(self, data_missing):
fill_value = data_missing[1]
data = data_missing.copy()
data._readonly = True
# by default fillna(copy=True), then this works fine
res_copy = data.fillna(fill_value, copy=True)
> assert res_copy[0] == fill_value
^^^^^^^^^^^^^^^^^^^^^^^^^
E AssertionError
pandas\tests\extension\base\missing.py:134: AssertionError
There was a problem hiding this comment.
That suggests that the fillna isn't actually working. That would indicate a real bug.
There was a problem hiding this comment.
i've used llm to debug and updated with some changes and restored as per your suggestion.
On running locally all tests were passing
|
Can you figure out the remaining test fialures |
|
sorry, i don't know if i should invest more time in this, as i've left with 2 PR's and two are closed, please feel free to close these. i don't want to create extra work for you to review . Thanks @jbrockmendel |
|
closed by #65179 |
Please let me know if my approach or fix needs any improvements . I’m open to feedback and happy to make changes based on suggestions.
Thank you!