Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DOC: better document Dtypes docstrings + avoid sphinx warnings #26067
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: better document Dtypes docstrings + avoid sphinx warnings #26067
Changes from all commits
acc47b9
e79d793
6620d84
44b8d09
8818681
f61ebc0
d43ee0f
82e53a5
84baf36
c8ae0c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have a round-trip on this test? shouldn't you also need
__getstate__
? or maybe not as you are setting _freq which is now good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__getstate__
is implemented in the super class:pandas/pandas/core/dtypes/dtypes.py
Lines 152 to 154 in 455a2cd
Added in 154a647
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's the round trip test that was failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, what if you move
_freq
into metdata then? (and so on for the other subclasses)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have been thinking about that as well.
_metadata
is also used for other things though (that are included in the EA interface, this__getstate__
is only for our internal dtypes). Eg for equality. For equality it would not matter iffreq
or_freq
is included, but if we would want to use it eg also for a default repr, then it need to be the public attributesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, yeah they are coupled. i don't really like having to be explicit about the pickle compat here.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree. Although it is consistent with how it is already done in the other dtypes (Categorical actually has the same), it would be nice to clean it up. I could also add a different
_metadata_pickles
list, so we can handle both getstate and setstate in the PandasExtensionDtype superclass. Of course that doesn't change the implementation, but it does reduce some duplication in the code.