CLN/INT: avoid getattr(obj, 'values', obj) idiom

[jorisvandenbossche]

We use this idiom quite a bit in the code base:

values = getattr(values, 'values', values)

One example:

pandas/pandas/core/algorithms.py

Line 123 in ad2e98c

values = getattr(values, 'values', values)

Personally, I find that this makes the code harder to reason about. What kind of object is values? What does the .values attribute return? (which depends on the object and dtype).

I think a more explicit coding can make things clearer. Sometimes I would find it clearer to just do:

if isinstance(values, ABCSeries):
    values = values.values

This is longer, yes, but then it is at least clear that it can potentially be a Series.

Although we should probably even try to avoid that algorithmic code is dealing with a mixture of Series, Index, EA, ndarray. I think, in general, we should nowadays be able to unpack containers in the beginning of / before algorithmic code so we know we only have either ndarray or EA.
Eg we could have an ensure_array that does such explicit checks and ensures the values are ndarray or EA.

cc @jreback @jbrockmendel @TomAugspurger

[jreback]

yep this is a good idea

[TomAugspurger]

Although we should probably even try to avoid that algorithmic code is dealing with a mixture of Series, Index, EA, ndarray. I think, in general, we should nowadays be able to unpack containers in the beginning of / before algorithmic code so we know we only have either ndarray or EA.
Eg we could have an ensure_array that does such explicit checks and ensures the values are ndarray or EA.

This is my preference. We have pandas.core.internals.construction.extract_array (should probably be moved).

[jreback]

ahh yes

if we rename to ensure_array and put in dtypes.common would be consistent

[jbrockmendel]

Generally on board, question about exactly what you have in mind:

if isinstance(values, ABCSeries):
    values = values.values

Do you have a strong opinion on this versus values._values or values.array? e.g. I'm pretty sure there are places where we do values = getattr(values, '_values', getattr(values, 'values', values)) which is... yikes.