BUG GH#40498 Fillna other missing values not modified #42981
Conversation
ghost
commented
Aug 11, 2021
ghost
commented
- closes BUG: Series.fillna is altering None values of unspecified columns #40498
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
|
Should be noted that these tests will not fail, even without my changes, because of an open issue - #18463. |
ghost
changed the title
mzeitlin11
added
Bug
Missing-data
ghost
mentioned this pull request
|
Hello @mikephung122! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-09-30 05:15:10 UTC |
pandas/core/generic.py
Outdated
| modification_index | ||
| ] | ||
| else: | ||
| value = value_map.reindex(self.index, copy=False) |
There was a problem hiding this comment.
why can't you just set copy=True for object type?
There was a problem hiding this comment.
This is the original logic that was present for all value dictionaries before I added the separate object type conditional.
I'm not sure why they chose to explicitly set copy=False since in both the old and new code we're replacing value with the reindexed version and the original value is not used anywhere else. In addition, unless every index is a key in the value dictionary I believe this will have no effect because - "A new object is produced unless the new index is equivalent to the current one and copy=False".
Will remove the unnecessary argument and return to the default since there's no clear value for it.
| modification_index | ||
| ] | ||
| else: | ||
| value = value_map.reindex(self.index) |
There was a problem hiding this comment.
if you don't add the copy does this just work e.g. do you still need L6314-6325
There was a problem hiding this comment.
Unfortunately the testing method we're using cannot differentiate the following:
result = pd.Series([1, None, 3, "four"])
expected = pd.Series([1, np.nan, 3, "four"])
tm.assert_equal(result, expected)
If you remove L6314-6325 the unit tests will pass but the original problem will still exist. I mentioned this on my initial Pull Request but these tests will not fail because of an open issue - #18463. I probably should've clarified if it's worth writing tests with other bugs in mind, or to write them as if the bug will be fixed eventually.
What do you think?
There was a problem hiding this comment.
.reindex should just work here. I am really not sure what you are trying to do.
There was a problem hiding this comment.
@jreback Sorry I might not be following, what did you want me to update here?
ghost
requested a review
from 
ghost
closed this
