Gh 36562 typeerror comparison not supported between float and str

[ssche]

• closes BUG: TypeError: '<' not supported between instances of 'float' and 'str' when using .combine_first with categorical index containing nan #36562
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[arw2019]

Thanks @SSHE for working on this!

Some comments. In the code they're almost all cosmetic. In the combine_first test you want to be using pandas._testing methods

[arw2019]

I would skip this comment.
In general we don't leave references to issues in the code, git/blame keeps track of that
You have a comment upstairs in sort_tuples that explains what it does so I think no need to repeat here

[arw2019]

I wouldn't explicitly assign these, just use them directly whenever they're needed
it's an O(1) lookup so this doesn't save any time but makes code a bit more verbose

[arw2019]

x_is_na and y_is_na

[arw2019]

why do we need (x_i_na and y_i_na)? Could one of them be null?

[ssche]

equality check - both of the values are the same (both na or both the same non-na value).

[arw2019]

could do this before checking for equality?

[arw2019]

I'd say no need to comment about consistency with numpy in the code (but thanks for noting it here!)

[ssche]

could do this before checking for equality?

sure, but why?

[arw2019]

generally speaking I wonder if the control flow can be simplified a bit here. you have a number of ifs but only two possible outputs

[ssche]

3 outcomes (=,<,>), but in combination with nan, they increase.

there are 9 ifs of which 2 are not relevant (shortcut before the loop, fall-though after the loop).

7 ifs remaining:

1. equality case (continue)
2. greater than case
3. smaller than case
4. x is nan, but y is not
5. y is nan, but x is not
6. xvec is smaller than yvec
7. yvec is smaller than xvec

I could merge some of the cases (with same return value), but that would compromise readability and require nan checks to be done earlier.

[ssche]

@arw2019 comments addressed (most of them), please review.

[jreback]

you don't need to do any of this

see safe_sort

[ssche]

what are you saying? what isn't needed?

this PR is changing safe_sort to accommodate mixed-type tuples

[jreback]

and how does this not do it now!

iirc this is adding a lot of non performing code for the purpose of fixing the error message?

[ssche]

and how does this not do it now!

now a TypeError will be raised in df.combine_first when the df contains a MultiIndex with nan and string values (sort_mixed, which is used now, fails in that case).

iirc this is adding a lot of non performing code for the purpose of fixing the error message?

in the event of all other (more performant) sorters failing, this (unarguably slower) sorter will be used. this should not compromise any other use case's performance, but at least makes the code in the ticket description succeed (slower, but at least not failing).

[ssche]

@jreback I moved the slow sort_tuples method so it really isn't called when any of the other (faster) sorters can work (2677166#diff-c8f3ad29eaf121537b999e88e9117f3e3702d0b818a67516da25093fe2890ce8R2114). please have another look and provide feedback.

[jreback]

yes let's change the name and please add typing for cmp_func and sort_tuples. (sort_mixed if you can as well :->)

[jreback]

can you simply try to remove the nan's from values and add them to the end is fine and just fix up sort_mixed, we do this in a number of places e.g. something like

In [204]: arr = np.array(['foo', 3, np.nan], dtype=object)                                                                                                                                                             

In [205]: mask = pd.notna(arr)                                                                                                                                                                                         

In [206]: arr.take(np.arange(len(arr))[mask])                                                                                                                                                                          
Out[206]: array(['foo', 3], dtype=object)

[ssche]

I started off with trying to fix sort_mixed and removed nans as you suggested and would have loved to use it instead. however the structure of the array doesn't allow for that (we are talking about a 1-d array of tuples, i.e. dtype object, not an N-d array which would allow for all those vector operations to be applicable).

here's the data of values that gets passed in to sort_tuples() when running the test I added:

In[2]: 
values
Out[2]: 
array([('b', 1), ('b', 2), ('c', 3), ('a', 4), ('b', 5), (nan, 6),
       ('a', 1), ('c', 1), ('d', 1)], dtype=object)
values[0]
Out[3]: ('b', 1)
values.shape
Out[4]: (9,)

I could convert value to a N-d array (and use sort_mixed), but that solution would come with its own overhead costs...

In[5]: 
np.asarray(list(values))
Out[5]: 
array([['b', '1'],
       ['b', '2'],
       ['c', '3'],
       ['a', '4'],
       ['b', '5'],
       ['nan', '6'],
       ['a', '1'],
       ['c', '1'],
       ['d', '1']], dtype='<U3')

your call.

[ssche]

any feedback on the above, @jreback?

[jreback]

i care about code complexity here, this is adding a lot, try this

[jreback]

i care about code complexity here, this is adding a lot, try this

[pep8speaks]

Hello @ssche! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

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

Comment last updated at 2020-11-03 23:54:13 UTC

[jreback]

thanks @ssche very nice!

[jbrockmendel]

@ssche two completely contradictory questions/requests

1. is there a reasonable way to do this without relying on core.internals?
2. could pd.MultiIndex.from_tuples de-duplicate some code by using to_arrays?

[ssche]

is there a reasonable way to do this without relying on core.internals?

My first attempt (1320ff1) wasn't using core.internals, as far as I recall, but it was deemed too complex.

could pd.MultiIndex.from_tuples de-duplicate some code by using to_arrays?

Maybe, what are you trying to achieve?

[jbrockmendel]

Maybe, what are you trying to achieve?

Just simplification/de-duplication

[ssche]

I don't know if this is still relevant (I forgot to respond), but I would argue that from_arrays should be preferred (and from_tuples delegating to from_arrays) as from_arrays could store type information better as it's accepting data in columnar format (instead of row-wise which from_arrays does).

Anyway, I hope you could proceed with your much appreciated simplification/de-dupe efforts...