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

ENH: infer_objects copy kwd #50096

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@mroeschke mroeschke added Dtype Conversions Unexpected or buggy dtype conversions Enhancement labels Dec 7, 2022
@mroeschke mroeschke added this to the 2.0 milestone Dec 7, 2022
@mroeschke mroeschke merged commit afca9f8 into pandas-dev:main Dec 7, 2022
@mroeschke
Copy link
Member

Thanks @jbrockmendel

Comment on lines +377 to +392
def convert(self: T, copy: bool) -> T:
def _convert(arr):
if is_object_dtype(arr.dtype):
# extract PandasArray for tests that patch PandasArray._typ
arr = np.asarray(arr)
return lib.maybe_convert_objects(
result = lib.maybe_convert_objects(
arr,
convert_datetime=True,
convert_timedelta=True,
convert_period=True,
)
if result is arr and copy:
return arr.copy()
return result
else:
return arr.copy()
return arr.copy() if copy else arr
Copy link
Member

Choose a reason for hiding this comment

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

@jbrockmendel sorry I was just about to ask - is there a test that hits this?

if not, then is it because the plan is to "kill arraymanager" anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

is there a test that hits this?

TestInferObjects.test_copy should hit this in the CI build specific to ArrayManager

the plan is to "#48880 (comment)" anyway

I'd like this, but have no plans to push for it.

@jbrockmendel jbrockmendel deleted the perf-infer_objects branch December 7, 2022 17:44
@jorisvandenbossche
Copy link
Member

@jbrockmendel for 1.5 we had a discussion related to new copy keywords (#48141) and ended up reverting some of those new copy keywords. Is there a specific reason to still add a copy keyword to infer_objects?

@jbrockmendel
Copy link
Member Author

Two big motivations. One was that we removed automatic inference in a few places, telling users to use infer_objects instead. Wanted to allow that operation to be cheap where possible. Second is that there was a bug in here in Block[Subclass].convert that was inconsistent about making copies. Fixing that bug required making more copies, and it's worth softening that blow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dtype Conversions Unexpected or buggy dtype conversions Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants