Skip to content

Datetime/Timedelta/Period Index min, argmin, max, argmax have axis kwarg #23081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jbrockmendel opened this issue Oct 11, 2018 · 9 comments · Fixed by #23206
Closed

Datetime/Timedelta/Period Index min, argmin, max, argmax have axis kwarg #23081

jbrockmendel opened this issue Oct 11, 2018 · 9 comments · Fixed by #23206
Labels
Datetime Datetime data dtype good first issue Needs Discussion Requires discussion from core team before further action

Comments

@jbrockmendel
Copy link
Member

Presumably shouldn't, or at least should raise if non-None.

@anjalibhavan
Copy link

Hi,
In which folder/file are these defined? And could you please provide some more detail?

@jbrockmendel
Copy link
Member Author

In which folder/file are these defined?

These methods are defined in pandas/core/indexes/datetimelike.py

And could you please provide some more detail?

DatetimeIndex/TimedeltaIndex/PeriodIndex are each 1-dimensional, so an axis argument doesn't really make sense. Also it is ignored if the user passes it, which is pretty much never good.

It's conceivable that the argument is there in order to keep the method's signature consistent with some other class's methods, but someone more knowledgeable than me will have to weigh in on that.

@datapythonista datapythonista added Datetime Datetime data dtype Effort Low Needs Discussion Requires discussion from core team before further action good first issue labels Oct 13, 2018
@datapythonista
Copy link
Member

@anjalibhavan the code we're talking about is for example: https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexes/datetimelike.py#L451

Personally I think we should get rid of them, and I don't think it's worth adding a deprecation warning, as the functions seem to accept *args and **kwargs and the code using that (if anyone ever did that) won't fail.

@gfyoung @jreback are we missing something here?

@5hirish
Copy link
Contributor

5hirish commented Oct 14, 2018

@datapythonista there is also take function at https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexes/datetimelike.py#L375 where axis argument has no usage. Let me know if that too has to be removed.
I just have removed the the axis argument and not args and kwargs

@gfyoung
Copy link
Member

gfyoung commented Oct 14, 2018

@datapythonista : We perform validation on args and kwargs on the very first line for compatibility with numpy. Thus, they serve a somewhat different purpose than in most other cases. As a result, I am hesitant to just "remove" the argument without some kind of deprecation first.

@datapythonista
Copy link
Member

thanks @5hirish, looks like it's the same case. @jbrockmendel I'll leave it to you whether to add it to the issue description or not, may be I'm missing something.

@gfyoung I didn't see that. Then it probably makes sense to deprecate first. But I don't think if there will be any code out there using a parameter that has no effect.

@gfyoung
Copy link
Member

gfyoung commented Oct 14, 2018

But I don't think if there will be any code out there using a parameter that has no effect.

@datapythonista : Well, better safe than sorry 🙂. That's all I'll say, since we seem to be in agreement that we should go with deprecation instead of removal.

@5hirish
Copy link
Contributor

5hirish commented Oct 14, 2018

@datapythonista Let me know I can revert the removals and add deprecation warnings if I understand correctly. I can see an example here: https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexes/datetimelike.py#L399

@jreback
Copy link
Contributor

jreback commented Oct 14, 2018

this is not possible because of numpy compatibility. This is also appropriately documented. Is there an issue there?

jbrockmendel added a commit to jbrockmendel/pandas that referenced this issue Oct 17, 2018
TomAugspurger pushed a commit that referenced this issue Oct 18, 2018
* validate min/max axis closes #23081

* Fix copy/paste indentation mistake

* Fix merge mixup
tm9k1 pushed a commit to tm9k1/pandas that referenced this issue Nov 19, 2018
* validate min/max axis closes pandas-dev#23081

* Fix copy/paste indentation mistake

* Fix merge mixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype good first issue Needs Discussion Requires discussion from core team before further action
Projects
None yet
6 participants