Skip to content

Conversation

@simonjayhawkins
Copy link
Member

mypy 0.750 has an experimental feature for static inference of annotations see https://mypy.readthedocs.io/en/latest/mypy_daemon.html#static-inference-of-annotations

have tried it out on functions in core.generic. The annotations add here are mainly the ones that matched the docstrings.

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Dec 3, 2019
@jreback jreback added this to the 1.0 milestone Dec 3, 2019

@property
def empty(self):
def empty(self) -> bool_t:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we be generally standarizing on using bool_t rather than bool?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO probably not necessary. probably only a few modules where this pattern is needed.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. @WillAyd

# Comparison Methods

def _indexed_same(self, other):
def _indexed_same(self, other) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

why do we have bool here and above but bool_t below? i expected to need bool_t throughout

also i think other should have same type as self here

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yea curious what the pattern here is; do only things after def bool need the alias? Or do you know if this is no longer an issue with Py36 onwards?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what appears to be the case. def bool is in the class namespace and doesn't effect the use of bool inside other methods see L8658. However, the functions signatures must be evaluated in the class namespace since any types added to the function signatures after the def bool need to use the alias.

Copy link
Member Author

Choose a reason for hiding this comment

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

also i think other should have same type as self here

hmm, the types should be representative of the runtime behaviour. should pd.Series()._indexed_same(pd.DataFrame()) raise?

>>> pd.__version__
'0.26.0.dev0+1155.ged20822a5'
>>>
>>> pd.DataFrame()._indexed_same(pd.DataFrame())
True
>>>
>>> pd.Series()._indexed_same(pd.DataFrame())
True
>>>
>>> pd.Series()._indexed_same(pd.Series())
True
>>>
>>> pd.DataFrame()._indexed_same(pd.Series())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\simon\pandas\pandas\core\generic.py", line 1400, in _indexed_same
    self._get_axis(a).equals(other._get_axis(a)) for a in self._AXIS_ORDERS
  File "C:\Users\simon\pandas\pandas\core\generic.py", line 1400, in <genexpr>
    self._get_axis(a).equals(other._get_axis(a)) for a in self._AXIS_ORDERS
  File "C:\Users\simon\pandas\pandas\core\generic.py", line 444, in _get_axis
    name = self._get_axis_name(axis)
  File "C:\Users\simon\pandas\pandas\core\generic.py", line 441, in _get_axis_name
    raise ValueError("No axis named {0} for object type {1}".format(axis, cls))
ValueError: No axis named columns for object type <class 'pandas.core.series.Series'>
>>>

Copy link
Member

Choose a reason for hiding this comment

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

I'd ask for bool_t to be made consistent across the module, totally OK with leaving other here as out of scope

@jbrockmendel
Copy link
Member

Question about bool vs bool_t consistency, not a blocker, otherwise LGTM

@jreback jreback merged commit 83672ad into pandas-dev:master Dec 4, 2019
@jreback
Copy link
Contributor

jreback commented Dec 4, 2019

thanks @simonjayhawkins happy to address the bool/bool_t issue later; we should likely just standardize on bool_t no?

@simonjayhawkins simonjayhawkins deleted the core.generic branch December 4, 2019 19:51
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Typing type annotations, mypy/pyright type checking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants