WIP: Panel Transpose Cleanup

[WillAyd]

Progress towards #25632

Prefacing this PR with the statement that this is pretty wonky, but I would consider that largely an artifact of cleaning up the existing code base.

Right now transpose is defined in NDFrame but is only used by DataFrame and Panel, as Series objects end up using a separate transpose defined in IndexOpsMixin.

There are duplicate calls to validate_transpose* with various signatures that aren't well defined, and everything is passed rather ambiguously via args and kwargs which makes validation rather difficult.

Existing validation is arguably rough. Here's a sample call and response on master:

>>> df = pd.DataFrame(np.ones((2, 2)))
>>> df.transpose('foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/williamayd/clones/pandas/pandas/core/frame.py", line 2643, in transpose
    nv.validate_transpose(args, dict())
  File "/Users/williamayd/clones/pandas/pandas/compat/numpy/function.py", line 55, in __call__
    self.defaults)
  File "/Users/williamayd/clones/pandas/pandas/util/_validators.py", line 218, in validate_args_and_kwargs
    validate_kwargs(fname, kwargs, compat_args)
  File "/Users/williamayd/clones/pandas/pandas/util/_validators.py", line 157, in validate_kwargs
    _check_for_default_values(fname, kwds, compat_args)
  File "/Users/williamayd/clones/pandas/pandas/util/_validators.py", line 69, in _check_for_default_values
    format(fname=fname, arg=key)))
ValueError: the 'axes' parameter is not supported in the pandas implementation of transpose()

As you can see the error message mentions the 'axes' parameter, but that isn't part of the signature of transpose and is instead ambiguously defined based off of args.

What I've done here is removed the Panel implementation and tried to make the NDFrame implementation more explicit. Because it's only used by DataFrame however, an alternate and arguably preferable implementation would be to raise NotImplementedError in NDFrame and handle everything in the DataFrame implementation once Panel is gone

[jschendel]

Can you make the default valid_axes=None and then handle as appropriate within the method? Mutable default arguments can lead to problems so probably best to avoid just to be safe.

[WillAyd]

Great callout! Do you think better to define here or even just move this code to the DataFrame class?

[jschendel]

Not super familiar with this, so don't want my opinion to carry too much weight, but I think it'd be cleaner to push this back to the DataFrame class and remove it from here entirely. If we're removing Panel.transpose then I think DataFrame is the only thing that uses this method, so seems more direct that way? Looks like Series/Index inherit from IndexOpsMixin and sparse things have their own implementation, so should be safe to remove unless I'm forgetting something?

[gfyoung]

We probably could move the "core" implementation into DataFrame, but I would think we should keep a skeleton implementation on transpose in NDFrame (even simple as raise NotImplemented, as transpose is arguably something that should be part of the implementation of anything that inherits from this base class).

[WillAyd]

Should also mention that this is somewhat of an "optional" cleanup. What brought me here was seeing Panel appear in the comment of validate_transpose_for_generic and then tracing that back to see that there were potentially duplicative calls to validate_transpose for DataFrame, whereas Panel solely relied on the former

At the end of the day Panel could simply be removed and no change, but I figure this is worth cleaning up to simplify is possible

[gfyoung]

@WillAyd : I understand what you're trying to do, but the motivation for having those "kwargs" and "args" in the signature was for numpy compatibility, which not ideal for us maintainers, was something that came up as an issue for users, hence why we implemented it.

If this is just cleanup, I would just make sure we can separate out this method so that we can cleanly take it out for Panel at the end of the day. Don't modify the signature at this point.

[WillAyd]

the motivation for having those "kwargs" and "args" in the signature was for numpy compatibility, which not ideal for us maintainers, was something that came up as an issue for users

Do you know if there was an issue attached to that? Without knowing the backstory just looking at the current implementation it seems like there's a lot of unnecessary code and strange error reporting with what's in place, so I think worth revisiting if maybe the compatibility piece was only applicable to older versions.

AFAICT the keyword arguments only ever make their way to the call to _construct_axes_from_arguments (which is pretty limited in scope) and enable copy to be passed, though that can be done with an explicit keyword argument as well

[gfyoung]

was only applicable to older versions.

@WillAyd : By compatibility, it means compatibility with numpy, period. This is not an issue regarding older versions. For reference, you can look at the current signature for transpose in 1.16, and you can see what I mean. That being said, happy to always look at this with fresh eyes and see how we can either improve or streamline.

Do you know if there was an issue attached to that?

It's been awhile, but if you'd like to dive into the history, you can go in a lot of directions from this PR I wrote then: #12810

[jreback]

not sure i understand the purpose of this PR. We can simply remove the use of Panel for transpose rather than actually change what you are doing.

[WillAyd]

Mostly just trying to simplify existing code. Can always tackle later.

…

Sent from my iPhone

On Apr 2, 2019, at 5:26 AM, Jeff Reback ***@***.***> wrote: @jreback requested changes on this pull request. not sure i understand the purpose of this PR. We can simply remove the use of Panel for transpose rather than actually change what you are doing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jreback]

I am not sure this does anything except add confusion. would rather just remove Panel methods / whole section that do this.