BUG: maybe_convert_objects mixed datetimes and timedeltas 
State: closed

- [X] corresponding [discussion](https://github.com/pandas-dev/pandas/issues/27417#issuecomment-512014358) 27417
- [X] tests added / passed
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

In order to stick to `maybe_convert_objects` realization it was needed to introduce `Seen().nat_` param. Since we want to distinguish following situations: were seen `NaT` or `datetime` or `timedelta`

NOTE: in current realization array full of `NaT` will be converted to `np.datetime64`.

Labels:
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff

Comments:
Have also ran following tests which compares new with old realization:
```python
check_types = [None, np.nan, pd.NaT,
               True,
               np.iinfo(np.int64).min-1, -1, 1, np.iinfo(np.int64).max+1,
               np.iinfo(np.uint64).max, np.iinfo(np.uint64).max+1,
               1.1, np.float_(1.1),
               1j, np.complex_(1j),
               datetime(2000, 1, 1),
               datetime(2000, 1, 1, 0, 0, 0, 0, timezone.utc),
               np.datetime64('2000-01-01'),
               timedelta(365),
               np.timedelta64(1, 's'),
               # Decimal(1.1),
               'object'
               ]
DT_TD_NaT_inds = [2] + list(range(14, 19))


def data_gen(perms, exclude_all_is_time=False):
    data = np.array([0]*N, dtype='object')
    for perm in perms:
        if exclude_all_is_time and all(ind in DT_TD_NaT_inds for ind in perm):
            continue
        for i in range(data.shape[0]):
            data[i] = check_types[perm[i]]
        yield data.copy()


N = 3
exclude_all_is_time = False
perms = itertools.product(list(range(len(check_types))), repeat=N)
datas = data_gen(perms, exclude_all_is_time)


class Tests:
    @pytest.mark.parametrize("data", datas)
    @pytest.mark.parametrize("safe", [1])
    # @pytest.mark.parametrize("conv_DT", [1])
    # @pytest.mark.parametrize("conv_TD", [1])
    # @pytest.mark.parametrize("safe", [0, 1])
    @pytest.mark.parametrize("conv_DT", [0, 1])
    @pytest.mark.parametrize("conv_TD", [0, 1])
    def test_maybe_convert_objects(self, data, safe, conv_DT, conv_TD):
        old = lib.maybe_convert_objects_OLD(data, 1, safe, conv_DT, conv_TD)
        new = lib.maybe_convert_objects(data, 1, safe, conv_DT, conv_TD)
        print('data:', data, data.dtype,
              '\nold:', old, old.dtype,
              '\nnew:', new, new.dtype)
        if isinstance(old, np.ndarray):
            tm.assert_numpy_array_equal(old, new)
        else:
            tm.assert_index_equal(old, new)
```
NOTE: have not parametrized by `safe` flag cause it concerns only `numeric`.

While running this tests with `exclude_all_is_time`, i.e. excluding tests with generated array fully consisting of `NaT` or `datetime` or `timedelta`- there is no errors all tests passed.

While `exclude_all_is_time` disabled (full test suite) got
```
Results (82.92s):
    31910 passed
      90 failed
```
Visually inspecting all of failed all of them concerns only BUG that is being fixed.
Failed [tests](https://pastebin.com/6HD89rzT). In `Captured stdout call` section of each of the tests there are input array - `data`, output of old/unfixed realization of algorithm - `old` and output of fixed realization - `new`.
Also have a question that is in general non relevant but don't understand where to ask. While adding `Seen.nat_` I've changed following functions as well:
https://github.com/pandas-dev/pandas/blob/2ae5152690cea598a7f377eb4cb264d6d42bb0cc/pandas/_libs/lib.pyx#L1045-L1051

My question is why in these functions there is no `.null_` nor `datetimetz_` nor `.object_`?
can you add an xref to the top post pointing back to the discussion (even though this doesnt close that issue)
needs tests.  even if they're not passing yet please push them

> My question is why in these functions there is no .null_ nor datetimetz_ nor .object_?

I don't know off the top of my head.  Probably check the blame to see where this was implemented?
> can you add an xref to the top post pointing back to the discussion (even though this doesnt close that issue)

Check please.

> needs tests. even if they're not passing yet please push them

But in order to test like I did I need both realization old and new in the `lib.pyx`. On my laptop I did tests -> remove old `maybe_convert_objects` -> push and make PR. How it should be tested then with old function removed?

In fact this is a problem I suppose. Because even my current test suite checking only against _existent realization outputs not predefined hand-made outputs_. And only reviewing comparisons make it feasible to catch errors. Filling correct answers for each test (even for `N=2` its already at least 3200 tests) is an overkill.
One of the solution (_strange_) for tests I see here is to dump tests output into file -> make function changes -> use same tests in order to comparing outputs. Then if changes will be approved this file with tests output should be updated to new outputs as well. 

Another one is to have 2 copy of function in the code - `old` one and `new` one in order to check `new` against the `old`. Pretty same as I did on my laptop. 
Disadvantage here is that old function must be used for test purposes _only_ (it maybe moved in other file - in test section for example). I think this is sane decision. 
> My question is why in these functions there is no .null_ nor datetimetz_ nor .object_?

@BeforeFlight : These were implemented initially to combat large number overflow issues.  Good call though to use them for purposes beyond that.
@BeforeFlight possibly helpful: coverage results for lib.pyx showing which lines are not hit in the tests:

pandas/_libs/lib.pyx                         1084     70    94%   307, 370, 651, 655, 658, 699-705, 707-710, 718-732, 828, 978, 983, 988, 1245-1246, 1268, 1445, 1519-1521, 1641, 1644-1646, 1696, 1718, 1889-1890, 1903, 1935, 1944, 1954-1956, 2006-2007, 2009-2010, 2022-2024, 2128-2131, 2135-2136, 2138-2139, 2143, 2145-2146, 2148-2153, 2191

grain of salt, since cython coverage results have been buggy in the past
@jbrockmendel so how should I add tests? Copy old function in tests section?
> so how should I add tests? Copy old function in tests section?

There should be some managably-sized set of cases that you were trying to fix here, i.e. that were previous wrong but are now right.  Write those tests specifically.  No need to try have a zillion tests.
> No need to try have a zillion tests

I get it. Agree that for this PR bunch of tests should be enough. 

But at the same time I should mark that inconsistencies of `maybe_convert_objects` that I've found - all was result of thoroughly testing. 
And another point to mention - this is core function (well I guess). That is why such complete test suite is needed in my opinion (not zillion tests - but _complete_ tests; count shouldn't matter). Besides this test suite may be ran only when this concrete function is amended.
Improving the thoroughness of the test suite is great, but for a bugfix PR, the tests should be specific to the bug being fixed.
Should I just remove this `NaT`s ambiguity for now and return `object`? Cause this concrete PR is somewhat not relative to this `NaT`s ambiguity problem.
> Should I just remove this NaTs ambiguity for now and return object? Cause this concrete PR is somewhat not relative to this NaTs ambiguity problem.

Between here and the issue you're bringing up a lot of questions and I'm having trouble keeping track of them.  To the extent that you can focus on one unambiguous bug at a time, that will make things easier for reviewers.
@BeforeFlight lgtm. does this have *any* user facing effects at all? meaning is there an incorrect Series/DataFrame construction currently?

Have to check (I'm not aware of all calling to `maybe_convert_object`) but if it used by some constructor directly - it might produce undesired results. The example that leads to this bug explains what I mean:
```python
import numpy as np
import pandas as pd
import pandas._libs.lib as lib

data = np.array([np.datetime64('2000-01-01'), np.timedelta64(1, 's')], dtype='object')
out = lib.maybe_convert_objects(data,1,1,1,1)
print(out, out.dtype)
```
Output:
```python
['2000-01-01T00:00:00.000000000' '1970-01-01T00:00:00.021389104'] datetime64[ns]
```

As I already mention at issue branch - datetime array is actually not initialized 2nd value at all (left it as it is after `np.empty`) -> might be any other in theory. 

Will try to reproduce it on some constructor.

__EDIT__
Straightforward `DataFrame`...`MultiIngex` runs smoothly - somehow overcome such input and returns `object`. Need some time to investigate it why. Will do.
No, seems like there is no any effects for user at all in current code state. The reason here is that in order to produce such bug both flags `convert_datetime` and `convert_timedelta` must be `True`. But I've search and visually inspect all `lib.maybe_convert_objects` calls in `pandas` folder - there is no such case. Only separate flags `convert_datetime` or `convert_timedelta` but not both at same time. 
thanks @BeforeFlight