FIX-#3896: Change Series.values to default to to_numpy(). #3979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: jeffreykennethli <jkli@ponder.io>
mvashishtha
previously approved these changes
Jan 14, 2022
Codecov Report
@@ Coverage Diff @@
## master #3979 +/- ##
===========================================
- Coverage 85.45% 47.20% -38.25%
===========================================
Files 199 187 -12
Lines 16531 15892 -639
===========================================
- Hits 14126 7502 -6624
- Misses 2405 8390 +5985
Continue to review full report at Codecov.
|
|
|
|
RehanSD
reviewed
Jan 18, 2022
RehanSD
requested changes
Jan 18, 2022
There was a problem hiding this comment.
I did some digging, and it looks like the error comes from this line, which expects non-empty Dataframes.
If we remove the call to super the error is fixed, and we are defaulting to to_numpy like @jeffreykennethli suggested, which seems to be what pandas intends in the future as well.
modin/pandas/series.py
Outdated
| def values(df): | ||
| return df.values | ||
|
|
||
| return self._default_to_pandas(values) |
There was a problem hiding this comment.
Suggested change
| return self._default_to_pandas(values) | |
| return self.to_numpy().flatten() |
There was a problem hiding this comment.
Done. I also removed the flatten() since presumably Series will already be 1-D, and flatten() is not necessary. Let me know if this is not a safe assumption.
Signed-off-by: jeffreykennethli <jkli@ponder.io>
| @@ -543,7 +543,7 @@ def values(self): # noqa: RT01, D200 | |||
| """ | |||
| Return Series as ndarray or ndarray-like depending on the dtype. | |||
| """ | |||
There was a problem hiding this comment.
was considering putting a:
UserWarning: 'Series.values` defaulting to to_numpy().
There was a problem hiding this comment.
This is not done for DataFrame.values.
Having this message for both DataFrame.values and Series.values is probably not super useful, except in the case where we're operating on an empty DataFrame/Series and we're already surfacing a
UserWarning: 'DataFrame.to_numpy` for empty DataFrame defaulting to pandas implementation.
in which case it will be confusing why DataFrame/Series using to_numpy and defaulting to pandas when the user is calling .values? (and the "values defaulting to to_numpy()" message would clarify).
jeffreykennethli
changed the title
RehanSD
requested changes
Jan 21, 2022
Signed-off-by: jeffreykennethli <jkli@ponder.io>
devin-petersohn
approved these changes
Jan 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: jeffreykennethli jkli@ponder.io
What do these changes do?
Modify Series.values to default to Series.to_numpy(), which conveniently handles the empty dataframe/series case from #3896.
In pandas/base.py, notice that if an attr is callable and self.empty, we default to pandas like in Series.to_numpy() which would fix our empty Series case from #3896.
Because to_numpy() and .values are supposed to have identical behavior (see pandas.Series.values and this PR), we can safely make Series.values call Series.to_numpy().
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/developer/architecture.rstis up-to-date