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

BUG: Arithmetic inplace ops not respecting CoW #51403

Merged
merged 6 commits into from
Feb 15, 2023

Conversation

phofl
Copy link
Member

@phofl phofl commented Feb 15, 2023

We should add tests for all op-possibilities in a follow up

if (
self.ndim == 1
and result._indexed_same(self)
and is_dtype_equal(result.dtype, self.dtype)
and not (
Copy link
Member Author

Choose a reason for hiding this comment

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

We could use iloc instead of checking for references, but would probably cost performance?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it is fine to use the manager method here (we can de-privatize if needed at some point)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, if you want to use an existing manager method that already takes care of this, on a SingleBlockManager, that would actually be self._mgr.setitem_inplace(slice(None), result._values)

That should translate to exactly the same (it only does an additional np_can_hold_element check, but I assume that should be cheap if the dtypes are equal, which is already checked above)

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this, switched over.

@phofl phofl mentioned this pull request Feb 15, 2023
5 tasks
@phofl
Copy link
Member Author

phofl commented Feb 15, 2023

greenish

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

Successfully merging this pull request may close these issues.

2 participants