BUG: set_index() and reset_index() not preserving object dtypes 
State: closed

* changes to `set_index()` and `ensure_index_from_sequences()` to pass current dtype of key through call stack onto Index constructor, rather than letting Index constructor infer the dtype. For now I just aimed to patch a single key param from `set_index()`, as that is the lowest hanging fruit for this issue I think.

* removed `maybe_convert_objects()` call from `_maybe_casted_values()`. This will preserve an `object` dtype on a `set_index().reset_index()` round-trip. I came to the exact same conclusion as the user in [this PR comment](https://github.com/pandas-dev/pandas/pull/27370#issuecomment-511051996), for context. This makes sense conceptually, if your index was an object dtype, it should be an object dtype as a column. Of course, this is somewhat subjective so let me know your opinions, but this is what I interpreted from the discussion on the git issue.

* both of these changes break quite a few existing tests, most of which are testing specifically for the exact opposite conditions that this PR is intended to patch **e.g.** tests that look for object dtypes being coerced to more-specific types. What's the process for handling failed tests for a change like this? Would you attempt to refactor all, or delete the ones where the logic is no longer something to test for? I also ran `pytest pandas` on a fresh pull of master and had 13 fails, with 108 failures on my branch specifically, so I am also curious what to do about those previously-failing tests, or what is acceptable for a merge.

- [x] closes #30517 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

Comments:
Hello @trevorbye! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:













There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2020-01-31 18:25:09 UTC
@jreback for visibility
> try to make an even smaller change this likely will break lots of things

Any suggestions for making these changes smaller? I tried to touch as little as possible, but not sure how I could slim it down further.

Or another question would be, what do you see breaking a lot? 

Thanks
@jreback @TomAugspurger any suggestions how to make this change smaller per your comment?

What are your thoughts on implementing this as non-default behavior? For example, adding a new optional param to both `set_index()` and `reset_index()` something like a boolean for `preserve_dtype`, that would enable the changes I've made here rather than make them the default. This would stop all these current tests from failing, and not break user's code. The tradeoff is the user would have to specify this param to get the new behavior, so I suppose that is a design question.

From the conversation in the [issue](https://github.com/pandas-dev/pandas/issues/30517) I had the impression it should be the new default behavior, but I could have misinterpreted. Either way let me know what you think the path forward is and I will work on it. Thanks
FYI @trevorbye you've commited some pickle files that are being left around by a some test (we haven't discovered which one yet).

I not sure what Jeff had in mind for making this smaller.

I think round-tripping the dtypes should be the default behavior. It not doing so today is a bug.

In theory, we could handle this in the Index constructor and just not have it infer the dtype when given a typed object. But that would probably be an API breaking change.
@TomAugspurger thanks, I'll get rid of those files. 

What is your best practice for refactoring/removing tests for a breaking change like this? There are quite a few failing, would you change the logic to reflect the new type inference (or lack thereof, in this case)? 

For Index constructor, if you call it yourself you can pass the dtype param which will disable the type coercion. But, when the constructor is called from **inside** `set_index()`, it doesn't pass dtype param. The change I made there simply grabs the type of the col you're trying to create an index from, and passes that on to the constructor and uses the logic already in place. 

Still learning the code base, but I'm wondering if there's a way to do this with less overall impact than the way I did it. 
I'm not sure. But if we're calling the dtype not being preserved a bug,
then we'll just need to update the tests that rely on the constructors
re-inferring dtypes.

On Mon, Jan 13, 2020 at 10:19 PM trevorbye <notifications@github.com> wrote:

> @TomAugspurger <https://github.com/TomAugspurger> thanks, I'll get rid of
> those files.
>
> What is your best practice for refactoring/removing tests for a breaking
> change like this? There are quite a few failing, would you change the logic
> to reflect the new type inference (or lack thereof, in this case)?
>
> For Index constructor, if you call it yourself you can pass the dtype
> param which will disable the type coercion. But, when the constructor is
> called from *inside* set_index(), it doesn't pass dtype param. The change
> I made there simply grabs the type of the col you're trying to create an
> index from, and passes that on to the constructor and uses the logic
> already in place.
>
> Still learning the code base, but I'm wondering if there's a way to do
> this with less overall impact than the way I did it.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/30870?email_source=notifications&email_token=AAKAOISLMZCB6XOKWHH6HWDQ5U4OBA5CNFSM4KE7USY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI3G6YY#issuecomment-573992803>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKAOIQYIG3QXRLHUHHMLY3Q5U4OBANCNFSM4KE7USYQ>
> .
>

@trevorbye yeah I agree with @TomAugspurger what we need to do here is actually fix this. This will by-definition break *some* tests which will need to be updated, some because we are specifically testing that functionaility, and some by happen-stance (we happened to construct the expected value with an inferred type).

So as I said above, my preference here is to leave the Index constructor alone, and simply pass in the dtype of what we are setting (now this will have to be slightly adjusted, so create a function to do this), e.g. a int* column -> np.dtype64 (as we don't support anything but Int64Index).



> @trevorbye yeah I agree with @TomAugspurger what we need to do here is actually fix this. This will by-definition break _some_ tests which will need to be updated, some because we are specifically testing that functionaility, and some by happen-stance (we happened to construct the expected value with an inferred type).
> 
> So as I said above, my preference here is to leave the Index constructor alone, and simply pass in the dtype of what we are setting (now this will have to be slightly adjusted, so create a function to do this), e.g. a int* column -> np.dtype64 (as we don't support anything but Int64Index).

@jreback thanks, I can refactor all those tests once we figure out the logic.

I think the change I made here is what you described; No change to Index constructor, I just passed it the current dtype. For reference, here is the main change I made, line 4266 in frame.py in my commit:

```python
if found:
    # get current dtype to preserve through index creation
    current_dtype = self.dtypes.get(col).type
```

This is then passed to `ensure_index_from_sequences(arrays, names, current_dtype)` (I created the new optional param here), which is then forwarded on to the Index constructor. I've done some debugging, and from what I'm seeing, the Index (and subclasses like Int64Index etc.) constructors will automatically handle unsupported index types if you attempt to create them. Here's an example:

```
In [1]: df = pd.DataFrame({'A': pd.Series([1, 2, 3], dtype=np.int32), 'B': [1, 2, 3]})

In [2]: print(df.dtypes.get('A').type)
Out [2]: <class 'numpy.int32'>

In [3]: print(df.set_index('A').index)
Out [3]: Int64Index([1, 2, 3], dtype='int64', name='A')
```

I ran the above code with my changes, and even though it's grabbing `np.int32` from the series type and using that as a param to the Index constructor dtype, it automatically detects that it's unsupported as an index type and coerces to int64.

So I don't think we need a new function to pre-convert these types before passing to Index constructors, as the logic is already there. Is this what you meant by a new function? Apologies if I'm going in circles, but to me it seems like the implementation we're discussing, and the implementation I made, are the same core logic, minus any edge cases or additional considerations I may be missing.

Thanks for your help! 
@jreback any feedback on my above comment? Want to see what you think before I go refactor a ton of tests 
will look soon; you have a lot of pickle files committed; remove those 
@jreback @TomAugspurger want to take another look? See my [comment](https://github.com/pandas-dev/pandas/pull/30870#discussion_r371485842) on your review, I tried your suggestion of doing `arrays.append(Index(col, dtype=col.dtype)`, but setting the dtype gets overridden by the outer constructor in that case.

There was some additional code I added as well to allow as many tests to pass as possible without altering them. I excluded datetime64 from this new logic, because there is a lot that breaks if you don't leave that as it is. Also added a try/catch for some edge cases that occur in some tests.
@jreback @jorisvandenbossche @WillAyd @TomAugspurger hi guys, can anyone take a look/see if we can merge? Thanks for your help
Focused on regressions from the 1.0 release right now. Should have a chance
to look next week.

On Fri, Jan 31, 2020 at 1:06 PM trevorbye <notifications@github.com> wrote:

> @jreback <https://github.com/jreback> @jorisvandenbossche
> <https://github.com/jorisvandenbossche> @WillAyd
> <https://github.com/WillAyd> @TomAugspurger
> <https://github.com/TomAugspurger> hi guys, can anyone take a look/see if
> we can merge? Thanks for your help
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/30870?email_source=notifications&email_token=AAKAOIT4VHU6W6B4AK54D2TRARZB3A5CNFSM4KE7USY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKPVQMI#issuecomment-580868145>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKAOIVLXTXFG4I7M3YPZWTRARZB3ANCNFSM4KE7USYQ>
> .
>

@trevorbye is this still active? Can you address comments and fix merge conflicts?
Closing as I think stale, but ping if you'd like to continue and can reopen