BUG: Handling NaN case before dtype conversion #40638 #41027
Conversation
|
Hello @Sudomarko! 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-04-19 03:59:09 UTC |
There was a problem hiding this comment.
Thanks for the pr @Sudomarko. First thing any pr needs is tests to make sure the changes actually work (which would catch the issues currently here).
In general, not sure why you're changing the method of filling missing values. That existing logic should be fine.
|
|
||
| if (copy or na_value is not lib.no_default) and result is self._ndarray: | ||
| if na_value is not lib.no_default: | ||
| temp.fillna(method="bfill") |
There was a problem hiding this comment.
Why bfill? Also, this line won't have any effect anyway because inplace=False by default.
There was a problem hiding this comment.
And na_value is no longer being used to fill
| @@ -355,9 +355,14 @@ def to_numpy( # type: ignore[override] | |||
| copy: bool = False, | |||
| na_value=lib.no_default, | |||
| ) -> np.ndarray: | |||
| result = np.asarray(self._ndarray, dtype=dtype) | |||
| temp = self | |||
There was a problem hiding this comment.
No reason for another temporary array, can just use result
|
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
|
@Sudomarko closing as stale. ping when ready to continue and i'll re-open. |
Handles the NA/NaN case before the dtype conversion