Skip to content

Conversation

@phofl
Copy link
Member

@phofl phofl commented Jan 9, 2023

@mroeschke mroeschke added this to the 2.0 milestone Jan 12, 2023
view = obj[:]
obj.sort_values(inplace=True, **kwargs)

assert np.shares_memory(obj.values, view.values)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is failing with ArrayManager (failing CI)

Copy link
Member

Choose a reason for hiding this comment

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

.values on a DataFrame is always a copy for ArrayManager, so the generic obj.values doesn't work here:

Suggested change
assert np.shares_memory(obj.values, view.values)
if isinstance(obj, DataFrame):
assert np.shares_memory(get_array(obj, "a"), get_array(view, "a"))
else:
assert np.shares_memory(obj.values, view.values)

Copy link
Member

Choose a reason for hiding this comment

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

You will have to use the same pattern below, so can probably simplify this if we give the Series a name so can use get_array in all cases.

Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 13, 2023

Choose a reason for hiding this comment

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

While testing this, I just pushed the changes I was doing locally, hopefully you weren't yet working on it.

using_copy_on_write
or using_array_manager
and isinstance(obj, DataFrame)
and pd.options.mode.copy_on_write
Copy link
Member

Choose a reason for hiding this comment

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

ArrayManager never uses Copy-on-Write (we should maybe error when setting those two options at the same time), so this check (and the or using_array_manager two lines above) shouldn't be needed. I suppose the reason it seemingly did give a copy is because of the .values giving a copy

@phofl
Copy link
Member Author

phofl commented Jan 13, 2023

Thx for fixing this, merged main to ensure that the dev builds get through

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.

3 participants