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

Revert "DEPR: is_copy arg of take (#30615)" #31032

Conversation

jorisvandenbossche
Copy link
Member

This reverts commit 7796be6.

See #27357 (comment) and #30615 (comment)

@jorisvandenbossche jorisvandenbossche added this to the 1.0.0 milestone Jan 15, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

-1 on this. the goal is to remove is_copy kwarg

i would rather fix take later if necessary, rather that redo this PR and fix take later with necessary

this is a step backwards and doesn’t accomplish anything

@jorisvandenbossche
Copy link
Member Author

i would rather fix take later if necessary, rather that redo this PR and fix take later with necessary

If we want to fix take later, why should we remove that behaviour of take now which we want to preserve later? (the fact that take is an actual copy)

BTW, I can also try to do a PR that actually fixes take. This is basically #30784, but in addition we would need to keep a private version of take that does the set_is_copy thing and ensure to use that private version where appropriate (i.e. mainly in the indexing code).
But then I first like to have some agreement on that general idea before putting time in it.

@jreback
Copy link
Contributor

jreback commented Jan 15, 2020

i would rather fix take later if necessary, rather that redo this PR and fix take later with necessary

If we want to fix take later, why should we remove that behaviour of take now which we want to preserve later? (the fact that take is an actual copy)

BTW, I can also try to do a PR that actually fixes take. This is basically #30784, but in addition we would need to keep a private version of take that does the set_is_copy thing and ensure to use that private version where appropriate (i.e. mainly in the indexing code).
But then I first like to have some agreement on that general idea before putting time in it.

that would be preferable

when private and public take were consolidated a while back this must have been broken

also include the tests for no setting with copy warning

thanks

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Jan 23, 2020

@jreback ok, did what is described above in #30784

So closing this PR then

@jorisvandenbossche jorisvandenbossche deleted the revert-take-copy branch January 23, 2020 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants