REF: define nanargminmax without values_for_argsort

[jbrockmendel]

No description provided.

[jorisvandenbossche]

This was done explicitly this way in the original PR #27801 (@jbrockmendel you actually explicitly questioned that use, which led to some discussion about it: #27801 (comment))

This gives a big slowdown, as doing a full argsort (to then only use a single index) is more expensive as argmin or argmax.

[jbrockmendel]

Thanks for linking back there.

But so, then we can simply expand this implmentation note in the docs to: you can either implement _values_for_argsort (and then argsort, argmin and argmax provide a default implementation based on those values), or implement argsort, argmin and argmax yourself. I don't think that is any problem?

I think a Technically Correct solution would look something like:

class ExtensionArray:
    def argsort(self, ...):
        raise NotImplementedError

    def argmax(self):
        return the_implementation_using_argsort

class VFAMixin:
    def _values_for_argsort(self):
        return the_implementation_in_master
    
    def argsort(self, ...):
        return the_implementation_in_master
    
    def argmax(self):
        return the_implementation_using_values_for_argsort

Getting there may be a hassle given backwards compatibility, but i think getting rid of the ambiguity as to values_for_argsort is worth it.

[jorisvandenbossche]

What's the benefit of your proposed scheme compared to the current situation? (i.e. what is quoted, assuming we properly document this)

[jbrockmendel]

I think its clearer. It would also make it much harder for us mess up and use _values_for_argsort in places we're not supposed to (even if we document it correctly, many a slip)

[jorisvandenbossche]

But also in your scheme we have a the_implementation_using_values_for_argsort function that will use _values_for_argsort, which was exactly the purpose of nanargminmax (the version from before this PR).

[jbrockmendel]

But it wouldn't be in the base class EA, which wouldn't have values_for_argsort at all

[jbrockmendel]

@jorisvandenbossche if you want to revert this i wont argue the point

[jreback]

prob should open a new issue so don't forget about this @jbrockmendel

[jbrockmendel]

opened #38201 to revert this

[jorisvandenbossche]

Ah, I already did that yesterday -> #38177

I think Jeff was asking for an issue about a better solution (eg your sketch above at #37815 (comment)). Now, I think we can use the existing #27218 for that.