-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: core.computation, mostly typing #29436
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
Changes from 2 commits
31d539b
dcbfe4e
78cd01e
c4c0ac6
ee3f208
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ def _check_ne_builtin_clash(expr): | |
|
||
Parameters | ||
---------- | ||
terms : Term | ||
expr : Term | ||
Terms can contain | ||
""" | ||
names = expr.names | ||
|
@@ -46,8 +46,9 @@ def __init__(self, expr): | |
self.aligned_axes = None | ||
self.result_type = None | ||
|
||
def convert(self): | ||
"""Convert an expression for evaluation. | ||
def convert(self) -> str: | ||
""" | ||
Convert an expression for evaluation. | ||
|
||
Defaults to return the expression as a string. | ||
""" | ||
|
@@ -75,10 +76,9 @@ def evaluate(self): | |
) | ||
|
||
@property | ||
def _is_aligned(self): | ||
def _is_aligned(self) -> bool: | ||
return self.aligned_axes is not None and self.result_type is not None | ||
|
||
@abc.abstractmethod | ||
def _evaluate(self): | ||
""" | ||
Return an evaluated expression. | ||
|
@@ -93,18 +93,19 @@ def _evaluate(self): | |
----- | ||
Must be implemented by subclasses. | ||
""" | ||
pass | ||
# mypy complains if we use @abc.abstractmethod, so we do use | ||
# AbstractMethodError instead | ||
from pandas.errors import AbstractMethodError | ||
|
||
raise AbstractMethodError(self) | ||
|
||
|
||
class NumExprEngine(AbstractEngine): | ||
"""NumExpr engine class""" | ||
|
||
has_neg_frac = True | ||
|
||
def __init__(self, expr): | ||
super().__init__(expr) | ||
|
||
def convert(self): | ||
def convert(self) -> str: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could also remove this method since super().convert returns a str. could this be a remnant of py2 compat? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll double-check. I tried removing all of the |
||
return str(super().convert()) | ||
|
||
def _evaluate(self): | ||
|
@@ -137,9 +138,6 @@ class PythonEngine(AbstractEngine): | |
|
||
has_neg_frac = False | ||
|
||
def __init__(self, expr): | ||
super().__init__(expr) | ||
|
||
def evaluate(self): | ||
return self.expr() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
import itertools as it | ||
import operator | ||
import tokenize | ||
from typing import Type | ||
from typing import Optional, Type | ||
|
||
import numpy as np | ||
|
||
|
@@ -40,7 +40,7 @@ | |
import pandas.io.formats.printing as printing | ||
|
||
|
||
def tokenize_string(source): | ||
def tokenize_string(source: str): | ||
""" | ||
Tokenize a Python source code string. | ||
|
||
|
@@ -68,7 +68,8 @@ def tokenize_string(source): | |
|
||
|
||
def _rewrite_assign(tok): | ||
"""Rewrite the assignment operator for PyTables expressions that use ``=`` | ||
""" | ||
Rewrite the assignment operator for PyTables expressions that use ``=`` | ||
as a substitute for ``==``. | ||
|
||
Parameters | ||
|
@@ -86,7 +87,8 @@ def _rewrite_assign(tok): | |
|
||
|
||
def _replace_booleans(tok): | ||
"""Replace ``&`` with ``and`` and ``|`` with ``or`` so that bitwise | ||
""" | ||
Replace ``&`` with ``and`` and ``|`` with ``or`` so that bitwise | ||
precedence is changed to boolean precedence. | ||
|
||
Parameters | ||
|
@@ -110,7 +112,8 @@ def _replace_booleans(tok): | |
|
||
|
||
def _replace_locals(tok): | ||
"""Replace local variables with a syntactically valid name. | ||
""" | ||
Replace local variables with a syntactically valid name. | ||
|
||
Parameters | ||
---------- | ||
|
@@ -135,7 +138,8 @@ def _replace_locals(tok): | |
|
||
|
||
def _clean_spaces_backtick_quoted_names(tok): | ||
"""Clean up a column name if surrounded by backticks. | ||
""" | ||
Clean up a column name if surrounded by backticks. | ||
|
||
Backtick quoted string are indicated by a certain tokval value. If a string | ||
is a backtick quoted token it will processed by | ||
|
@@ -303,7 +307,8 @@ def f(self, *args, **kwargs): | |
|
||
|
||
def disallow(nodes): | ||
"""Decorator to disallow certain nodes from parsing. Raises a | ||
""" | ||
Decorator to disallow certain nodes from parsing. Raises a | ||
NotImplementedError instead. | ||
|
||
Returns | ||
|
@@ -324,16 +329,17 @@ def disallowed(cls): | |
|
||
|
||
def _op_maker(op_class, op_symbol): | ||
"""Return a function to create an op class with its symbol already passed. | ||
""" | ||
Return a function to create an op class with its symbol already passed. | ||
|
||
Returns | ||
------- | ||
f : callable | ||
""" | ||
|
||
def f(self, node, *args, **kwargs): | ||
"""Return a partial function with an Op subclass with an operator | ||
already passed. | ||
""" | ||
Return a partial function with an Op subclass with an operator already passed. | ||
|
||
Returns | ||
------- | ||
|
@@ -813,18 +819,27 @@ class Expr: | |
parser : str, optional, default 'pandas' | ||
env : Scope, optional, default None | ||
truediv : bool, optional, default True | ||
level : int, optional, default 2 | ||
level : int, optional, default 0 | ||
""" | ||
|
||
def __init__( | ||
self, expr, engine="numexpr", parser="pandas", env=None, truediv=True, level=0 | ||
self, | ||
expr, | ||
engine: str = "numexpr", | ||
parser: str = "pandas", | ||
env=None, | ||
truediv: bool = True, | ||
level: int = 0, | ||
): | ||
self.expr = expr | ||
self.env = env or Scope(level=level + 1) | ||
self.engine = engine | ||
self.parser = parser | ||
self.env.scope["truediv"] = truediv | ||
self._visitor = _parsers[parser](self.env, self.engine, self.parser) | ||
self._visitor = _parsers[parser]( | ||
self.env, self.engine, self.parser | ||
) # type: Optional[BaseExprVisitor] | ||
assert isinstance(self._visitor, BaseExprVisitor), type(self._visitor) | ||
Comment on lines
+841
to
+842
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that the type should be Optional or that the type annotation should be added here. adding type annotations directly to _parsers...
negates the need for the assert, but does mean that the type annotations added later in this PR in pandas/core/computation/pytables.py need to be revisted. |
||
self.terms = self.parse() | ||
|
||
@property | ||
|
@@ -834,10 +849,10 @@ def assigner(self): | |
def __call__(self): | ||
return self.terms(self.env) | ||
|
||
def __repr__(self): | ||
def __repr__(self) -> str: | ||
return printing.pprint_thing(self.terms) | ||
|
||
def __len__(self): | ||
def __len__(self) -> int: | ||
return len(self.expr) | ||
|
||
def parse(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy can afford additional checking since it has some understanding of abstract classes. so I'm not 100% happy with this, if it sets a precedent for dealing with
Cannot instantiate abstract class 'AbstractEngine' with abstract attribute '_evaluate'
style mypy errors.in this case,
also passes mypy without changes to the code and only type annotations added.
It also means we retain the modern Python functionality for dealing with abstract methods instead of homegrown.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The homegrown doesn't really bother me since we use it in a lot of places. But annotations is an area where I'm going to defer to you pretty consistently.
Is this supported in py35? I rebel at this pattern because I always think "mypy should be able to figure this out" (which is only true if this were some kind of frozen_dict, but still).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#29212 seems to have gone stale. so will need to use the py3.5 syntax
It is required in a lot of places and in this case on my branch I currently have
but can't remember why it was needed, but it looks like it will need to be updated to account for the abstract method if not done in this PR.