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

DOC: Add note for copy keywords for Copy-on-Write #56033

Merged
merged 6 commits into from
Nov 22, 2023

Conversation

phofl
Copy link
Member

@phofl phofl commented Nov 17, 2023

  • 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.

Starting with one method to figure out wording. Will add for the remaining ones after this one

@@ -6428,6 +6428,15 @@ def astype(
Return a copy when ``copy=True`` (be very careful setting
``copy=False`` as changes to values then may propagate to other
pandas objects).

.. note::
Copy link
Member

@lithomas1 lithomas1 Nov 20, 2023

Choose a reason for hiding this comment

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

Can you make this warning a shared docstring?

Copy link
Member

Choose a reason for hiding this comment

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

Personally I find shared docstrings quite annoying (and untransparant, difficult to understand), so while it's nice to deduplicate and only have 1 place to update this note, I am also fine with simply copy-pasting this note multiple times once we have finalized its wording.

(certainly for methods that right now don't yet make use of shared docstrings)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I don't like them either

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I might want to say something like: "in the future this will no longer copy by default, so you don't need to specify this keyword", but the problem is in being both succinct and accurate, because of course that isn't fully true, we will copy later on. Which is what your "lazy copy" covers, but then I don't know how many people will fully understand that.

Do we want to also make use of this note to mention "you can already get that future behaviour by enabling CoW, see ...` ?

pandas/core/generic.py Outdated Show resolved Hide resolved
pandas/core/generic.py Outdated Show resolved Hide resolved
pandas/core/generic.py Outdated Show resolved Hide resolved
phofl and others added 4 commits November 20, 2023 15:25
Co-authored-by: Joris Van den Bossche <[email protected]>
Co-authored-by: Joris Van den Bossche <[email protected]>
Co-authored-by: Joris Van den Bossche <[email protected]>
@phofl
Copy link
Member Author

phofl commented Nov 20, 2023

I've added the enable cow already (big fan of this) and added a defers the copy to the lazy copy mechanism (we also link to the cow docs where this is explained in more detail)

@phofl phofl merged commit 4cd6d7e into pandas-dev:main Nov 22, 2023
40 checks passed
@phofl
Copy link
Member Author

phofl commented Nov 22, 2023

going ahead with the others now

@phofl phofl deleted the copy_keyword_docs branch November 22, 2023 22:16
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