REF: boilerplate for ops internal consistency
State: closed

Progress towards #23853, with more progress available pending resolution of #27911.


Labels:
name: Refactor
description: Internal refactoring of code

Comments:
needs a rebase
> IIRC there was a PR that tried to do this, can you refernce it and/or the associated issues?

@jreback I think https://github.com/pandas-dev/pandas/pull/24846 was the other issue. That targeted https://github.com/pandas-dev/pandas/issues/23853. (I think this doesn't quite close it @jbrockmendel?)
> I think #24846 was the other issue. That targeted #23853. (I think this doesn't quite close it @jbrockmendel?)

Yes, #23853 was the original issue.  This goes a long way towards addressing it, but I think there is more boilerplate we can roll into this* in follow-up(s), so doesn't close the issue entirely.

#24846 was another PR aimed at the same issue, but I was -1 on the implementation there for reasons described [here](https://github.com/pandas-dev/pandas/pull/24846#issuecomment-469041058).

\* Things we can hopefully roll into this boilerplate eventually include:
 - standardize how we treat list-like `other`, i.e. when to do `other = np.asarray(other)`
 - standardize length-mismatch checks
> Is there a need to do the unpack_and_defer as a decorator instead of a helper function that is called in the beginning of the main wrapper function?

It could work either way.  I'm partial to the decorator because it is a one-linear instead of a three-linear

```
ni, other = unpack_and_defer(self, other):
if ni:
    return NotImplemented
```

but if there's a consensus against the decorator it's not a hill I want to die on.
Ah, I didn't think of the potential return value, that's a good reason to have it as a decorator. 
Would you then want to make it a single wrapper (and not the double wrapping to prevent indentation as you do now) to still somewhat limit the wrapping?
> Would you then want to make it a single wrapper (and not the double wrapping to prevent indentation as you do now) to still somewhat limit the wrapping?

Not especially.  As above, I prefer the way it is here, but if there is consensus in the other direction, not a big deal.
I found the _unpack_and_defer and unpack_and_defer a bit strange, but not a
huge deal.

On Tue, Sep 17, 2019 at 11:24 AM jbrockmendel <notifications@github.com>
wrote:

> Would you then want to make it a single wrapper (and not the double
> wrapping to prevent indentation as you do now) to still somewhat limit the
> wrapping?
>
> Not especially. As above, I prefer the way it is here, but if there is
> consensus in the other direction, not a big deal.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/28037?email_source=notifications&email_token=AAKAOISE7TRDLFU4YCF3FGDQKEAFDA5CNFSM4INYONKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD65DB5I#issuecomment-532295925>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOITISKZJUYTSHG5XKSTQKEAFDANCNFSM4INYONKA>
> .
>

An option for the future could make the `op_name` arg unnecessary (and therefore the separate steps unpack_and_defer -> _unpack_and_defer) by pinning it for the subset of methods for which it is not already correct:

```
@unpack_and_defer
@rename(op_name)
def generated_func(self, other): ...

instead of

@unpack_and_defer(op_name)
def generated_func(self, other): ...
```
Other question for my understanding: the "unpack" in the decorator name, does that point to the unpacking of zerodim array? Or also to (future?) unpacking of Series/Index to array?
> the "unpack" in the decorator name, does that point to the unpacking of zerodim array?

Yes.  I'm open to other names if you have a strong preference.

> Or also to (future?) unpacking of Series/Index to array?

I hadn't thought of that, but tentatively like it.  The main thing I have in mind for extending this in the future is doing a) non-array listlike coercion and b) length-match checks since we're not very consistent about either of these.
If this is not actually unpacking our containers, I think another name would be better (I actually thought all the time those methods would be for this purpose)

Just `unpack_zerodim` instead of `unpack` can already help?
> Just unpack_zerodim instead of unpack can already help?

Sure.
@jbrockmendel status of this
> status of this

there was a request to name something differently that i need to get around to, otherwise this should be ready (havent rebased in a while, so should do that too)
> > status of this
> 
> there was a request to name something differently that i need to get around to, otherwise this should be ready (havent rebased in a while, so should do that too)

this PR lgtm. I can't find that request, what was it?
> I can't find that request, what was it?

Addressed in a commit yesterday.  It was to rename "unpack_and_defer" to "unpack_zerodim_and_defer"
thanks @jbrockmendel