Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEPR: Deprecate ravel #56053

Merged
merged 15 commits into from
Nov 27, 2023
Merged

DEPR: Deprecate ravel #56053

merged 15 commits into from
Nov 27, 2023

Conversation

phofl
Copy link
Member

@phofl phofl commented Nov 18, 2023

@phofl phofl added the Deprecate Functionality to remove in pandas label Nov 18, 2023
@phofl phofl added this to the 2.2 milestone Nov 18, 2023
@jorisvandenbossche jorisvandenbossche changed the title DEP: Deprecate ravel DEPR: Deprecate ravel Nov 20, 2023
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Can you also add a .. deprecated:: 2.2.0 note to the docstring?

array([1, 2, 3])
"""
warnings.warn(
"Series.ravel is deprecated. Use numpy.ravel directly.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we should also point to to_numpy() in case people use this to essentially get the numpy series (the "ravel" aspect never does something, since Series is already 1D)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just "dont do it bc it isnt necessary"? (also should we deprecate Index.ravel?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just "dont do it bc it isnt necessary"?

That's not necessarily correct. If you rely on the current return value, i.e. if your code requires that the object is a 1D numpy array, and you currently use ravel for that, you do need to replace ravel with a conversion to numpy.
(and that's actually how we were implicitly using it internally)

warnings.warn(
"Series.ravel is deprecated. Use numpy.ravel directly.",
FutureWarning,
stacklevel=find_stack_level(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stacklevel=find_stack_level(),
stacklevel=2,

For a simple case like this where the warning is directly raised in the top-level method, I prefer setting it explicitly, which avoids potential hickups with the function (I think we had perf issues in certain cases, and this also avoids getting a wrong stacklevel in case we forget a use case of ravel in our own code base)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@@ -309,15 +309,17 @@ def _add_margins(

row_names = result.index.names
# check the result column and leave floats

def _cast_types(x, dtype):
return maybe_downcast_to_dtype(x._values, dtype)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do that inside maybe_downcast_to_dtype instead? That function is also used in some other places (I don't know if it is guaranteed to be called with a non-Series in those other places, but our test coverage might also be lacking on that aspect)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

array([1, 2, 3])
"""
warnings.warn(
"Series.ravel is deprecated. The underlying array is already 1D, so "
"ravel is not necessary.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"ravel is not necessary.",
"ravel is not necessary. Use `to_numpy()` for conversion to a numpy array instead.",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@@ -846,6 +846,8 @@ def ravel(self, order: str = "C") -> ArrayLike:
"""
Return the flattened underlying data as an ndarray or ExtensionArray.

.. deprecated:: 2.2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the content of the warning message here as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@jorisvandenbossche jorisvandenbossche merged commit 3934e56 into pandas-dev:main Nov 27, 2023
40 checks passed
@jorisvandenbossche
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEPR: Deprecate Series.ravel
3 participants