-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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: make_block #57754
DEPR: make_block #57754
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM cc @jorisvandenbossche
I have been working on a branch for pyarrow testing an alternative |
BTW I posted some comments on the issue last week: #56815 |
@jorisvandenbossche with #58197 in are we good to move forward here? |
If there are no comments I plan to merge next week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbrockmendel apologies for the very slow follow-up here!
Added a few minor questions, but yes, in general I am fine with moving forward with this now (pyarrow 16.0 is released with the change for this, so that's the main thing I would have wanted to wait on)
pandas/core/internals/api.py
Outdated
"make_block is deprecated and will be removed in a future version. " | ||
"Use public APIs instead.", | ||
DeprecationWarning, | ||
stacklevel=find_stack_level(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stacklevel=find_stack_level(), | |
2, |
pandas/core/internals/api.py
Outdated
warnings.warn( | ||
# GH#56815 | ||
"make_block is deprecated and will be removed in a future version. " | ||
"Use public APIs instead.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to point to the new pd.api.internals.create_dataframe_from_blocks
? I know we don't want to encourage that, but TBH I assume this is used by few libraries for a specific reason anyway (exactly the use case for which the pd.api.internals can be used for), and the docstring of that function also asks the user to raise an issue about it
with warnings.catch_warnings(): | ||
warnings.filterwarnings( | ||
"ignore", | ||
"make_block is deprecated", | ||
DeprecationWarning, | ||
) | ||
|
||
return feather.read_feather( | ||
handles.handle, columns=columns, use_threads=bool(use_threads) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this? (the fix is in a released pyarrow, so with latest of both you won't see any warning. Although of course when testing with an older pyarrow you will see it, but I am not sure we added such warning suppressions for other deprecations like DataFrame(mgr)?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont need it, no. we need to catch this either here or filterwarnings in a bunch of test places. so i prefer to catch it one place, but OK either way
Updated, I think addressing all comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Looks good
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.