Skip to content
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

TYP: define Index.any, Index.all non-dynamically #36929

Merged

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

xref #31160, cc @simonjayhawkins

This unearths 2.5 issues:

  1. Index.any/all says that we pass *args/**kwargs to np.any/all, but we don't
    1b) the RangeIndex methods don't have args/kwargs at all
  2. the subclasses that disable any/all don't totally make sense. In particular it seems like Float64Index should support it and CategoricalIndex should support it iff self.categories supports it

@jreback jreback added the Numeric Operations Arithmetic, Comparison, and Logical operations label Oct 7, 2020
@jreback jreback added this to the 1.2 milestone Oct 7, 2020
@jreback
Copy link
Contributor

jreback commented Oct 7, 2020

can you rebase

False
"""
# FIXME: docstr inaccurate, args/kwargs not passed
self._maybe_disable_logical_methods("any")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have a numpy validation routine for this and for all? eg. https://github.com/pandas-dev/pandas/blob/master/pandas/compat/numpy/function.py

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jbrockmendel generally lgtm

or is_float_dtype(self.dtype)
):
# This call will raise
make_invalid_op(opname)(self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be done on the subclasses. more repetitive but would be easier to follow I think.

although, does disabling on subclasses obey LSP?

maybe a mixin that classes can opt into, to ensure that the base class or other code does not rely on any/all being available.

This would mean a base class for Index, to allow any/all on object Indexes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ATM we disable on subclasses (though by calling cls.add_disabled_whatever()), and i thought it was clearer to do this in just one place, but if people disagree im fine with going the other direction.

I don't think it's that big a deal because I expect to change it before long since, as mentioned in the OP, i think several of the currently-disabled methods should be enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with perhaps revisiting after some of the currently-disabled methods are enabled.

@simonjayhawkins
Copy link
Member

This unearths 2.5 issues:

  1. Index.any/all says that we pass *args/**kwargs to np.any/all, but we don't
    1b) the RangeIndex methods don't have args/kwargs at all
  2. the subclasses that disable any/all don't totally make sense. In particular it seems like Float64Index should support it and CategoricalIndex should support it iff self.categories supports it

@jbrockmendel maybe open issues for these and @jreback comment #36929 (comment)

@simonjayhawkins simonjayhawkins merged commit c04e231 into pandas-dev:master Oct 8, 2020
@simonjayhawkins
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the typ-31160-index-logical branch October 8, 2020 15:26
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Numeric Operations Arithmetic, Comparison, and Logical operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants