-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Use repr of fill_value in SparseDtype repr #34352
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
Conversation
9e7d905
to
8c6546e
Compare
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.
Good idea!
@@ -402,6 +402,16 @@ class TestPrinting(BaseSparseTests, base.BasePrintingTests): | |||
def test_array_repr(self, data, size): | |||
super().test_array_repr(data, size) | |||
|
|||
def test_fillna_repr(self): |
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.
Can you put this test in one of the files in tests/arrays/sparse/..
(eg in test_dtype.py)?
(trying to keep those extension tests only the shared common ones inherted from the base tests, any custom test goes into tests/arrays
instead of tests/extension/
)
Ok, I've moved the tests. |
looks good can you add a release note (sparse bug fix section is ok). ping on green. |
Thanks! |
btw if would be helpful to survey other dtypes (mainly category) to see if we are consistent for various types |
Non of the other dtypes have the concept of a "fill_value" (or print another value in its dtype repr), so I am not sure what should be checked? |
of course; i am speaking generally we expose other things in the repr eg categories |
The CategoricalDtype is fine:
Categorical itself is not:
but that is actually already being handled in #34222 |
This proposes changing the StringDtype repr to show the fill_value's repr rather than its string representation: