TST (string dtype): clean-up assorted xfails #60345
Conversation
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
(cherry picked from commit e7d1964)
|
Manual backport -> #60349 |
| assert np.shares_memory(arr, result) is False | ||
| else: | ||
| assert np.shares_memory(arr, result) is True | ||
|
|
||
| result = obj.to_numpy(copy=False) | ||
| if using_infer_string and arr.dtype == object: | ||
| if using_infer_string and arr.dtype == object and obj.dtype.storage == "pyarrow": |
There was a problem hiding this comment.
Why if a user specifically requests not to make a copy are we converting the numpy array to a pyarrow arrow baked string array for what is an immutable index? There are other performance benefits?
There was a problem hiding this comment.
It's the other way around, the pyarrow array (stored under the hood in obj) is being converted to a numpy array, and that can just never be done without a copy (different memory layout)
There was a problem hiding this comment.
the first line of the test is obj = pd.Index(arr, copy=False)
so if we have a numpy arr and specify copy=False for the immutable index we get a pyarrow backed index and a copy is made? and then the .to_numpy() method makes another copy?
>>> pd.options.future.infer_string = True
>>> arr = np.array(["a", "b", "c"], dtype=object)
>>> arr
array(['a', 'b', 'c'], dtype=object)
>>>
>>> idx = pd.Index(arr, copy=False)
>>> idx
Index(['a', 'b', 'c'], dtype='str')
>>> so the question is perhaps should idx = pd.Index(arr, copy=False) return an Index with a string dtype, perhaps raise like numpy now do for __array__ when a copy can't be made or is this a moot point when CoW is extended to the Indexes as the copy keyword would be irrelevant?
There was a problem hiding this comment.
Ah, sorry, I assumed you were commenting on what is being tested here, i.e. the obj.to_numpy(copy=False) copying or not.
For pd.Index(arr, copy=False): in general our copy keywords in constructors are not strict, but only mean to avoid a copy at "best effort" (e.g. also if you pass a python list, it will make a copy regardless of that keyword). If we would want to change that more generally, that's a bigger topic to discuss.
this a moot point when CoW is extended to the Indexes as the copy keyword would be irrelevant?
It's certainly not a moot point, because with CoW we actually copy more often on input with non-pandas objects. Although it seems we didn't make that change for Index(..), where the default is still copy=False
| def test_union_with_na_when_constructing_dataframe(): | ||
| # GH43222 | ||
| series1 = Series( | ||
| (1,), | ||
| index=MultiIndex.from_arrays( | ||
| [Series([None], dtype="string"), Series([None], dtype="string")] | ||
| [Series([None], dtype="str"), Series([None], dtype="str")] |
There was a problem hiding this comment.
So this does fix the test. Is the behavior if this change to the test is not made correct?
i.e. the series1.index.dtypes are object and the series2.index.dtypes are str and the resulting dtype for the columns index using the DataFrame constructor is object. Would we not expect the DataFrame constructor to return a str index for the columns in this case?
There was a problem hiding this comment.
perhaps related to #60338?
There was a problem hiding this comment.
i.e. the
series1.index.dtypesareobjectand theseries2.index.dtypesarestrand the resulting dtype for the columns index using the DataFrame constructor isobject.
With the above fix (and when infer_string is enabled), the test uses str dtype for the index levels of both series1 and series2, and then also the expected result gets created with that.
So it's only testing that the NaNs are properly matched when creating the rows from Series objects with a MultiIndex, it does not test having different dtypes in series1 vs series2.
That's also something we could test, though (and then if you have object dtype index and str dtype index, one would expect the result to be object dtype index (since that is the "common" dtype), but was not how the test was currently set up.
perhaps related to #60338?
I suppose not because here we don't actually have empty objects. Both series1 and series2 have data and have a non-empty index.
| request.applymarker(pytest.mark.xfail("object and strings dont match")) | ||
| request.applymarker( | ||
| pytest.mark.xfail( | ||
| reason="TDOD(infer_string) object and strings dont match" |
There was a problem hiding this comment.
There's a typo here that may mean this gets missed when grepping the TODOs.
There was a problem hiding this comment.
Ah, good catch! Yes, I do grep for that so good to fix that typo
An bunch of assorted xfails that were no longer needed or had a trivial fix
xref #54792