BUG: Preserve name in Index.astype #32036
Conversation
| from pandas import Index | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
this is not very comprehensive. just hook into the current astype tests and check for this.
There was a problem hiding this comment.
there are llikely many more paths that are not exactly correct.
There was a problem hiding this comment.
@dsaxton this is the much more important point
There was a problem hiding this comment.
I'm not sure what's meant by the current astype tests since there are lots of them. Would extending the parameterization work for being more comprehensive?
There was a problem hiding this comment.
no but i would rather simply add a check i. the current astype tests
there are many many cases and this doesn’t begin to cover
There was a problem hiding this comment.
Not a bad idea, was struggling with how to test this exhaustively when certain conversions won't even be possible
There was a problem hiding this comment.
@dsaxton did you decide to stick with whats currently here, or have another approach in mind?
There was a problem hiding this comment.
@jbrockmendel I think I'll try your idea for these tests
There was a problem hiding this comment.
With this approach I'm assuming we'd skip the MultiIndex cases in the indices fixture, since the name attribute isn't so interesting (we care about the names)? Those seem to be all the test failures.
There was a problem hiding this comment.
that or check that the names are retained. If this isnt for just the base Index class, then it probably belongs in tests.indexes.test_common
pandas/tests/indexes/test_common.py
Outdated
| @@ -337,3 +337,25 @@ def test_has_duplicates(self, indices): | |||
| idx = holder([indices[0]] * 5) | |||
| assert idx.is_unique is False | |||
| assert idx.has_duplicates is True | |||
|
|
|||
| @pytest.mark.parametrize("dtype", ["int64", "uint64", "float64", "category"]) | |||
There was a problem hiding this comment.
was this the one where adding more dtypes here led to 90k tests?
There was a problem hiding this comment.
I don't think it's too many; added a couple more. Let me know if there are others you think I should include.
@jbrockmendel Looks like there's an unrelated CI failure, but probably just wait for that to be fixed? |
|
Yah, hopefully that will be fixed by #32630. |
@jbrockmendel Green now, thanks for the review |
|
cc @jreback i think this is good to go |
| else: | ||
| indices.name = "idx" | ||
|
|
||
| try: |
There was a problem hiding this comment.
can you add a comment here on why you have this try/except
|
@jbrockmendel if you have any more comments |
|
im a happy camper |
|
lgtm. can you merge master, and ping on green (just to make sure it respects all of the recent merges) |
@jreback Merged master + green |
|
hmm somthing failing here, pls merge master and see |
|
thanks @dsaxton |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff