CLN/TYP: address TODOs, ignores #44308
Conversation
jbrockmendel commented Nov 4, 2021
jbrockmendel commented - closes #xxxx
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
simonjayhawkins left a comment
simonjayhawkins left a comment There was a problem hiding this comment.
Thanks @jbrockmendel generally LGTM.
| # error: Incompatible return value type (got "Tuple[ndarray, ndarray]", | ||
| # expected "Tuple[ndarray, ExtensionArray]") | ||
| return codes, uniques # type: ignore[return-value] | ||
| uniques_ea = self._from_factorized(uniques, self) |
There was a problem hiding this comment.
the return type annotation is missing for _from_factorized so this resolves to Any
maybe could change here but the variable name would still need to changed anyway so could also be follow-up since _quantile_ea_compat would also need fixing.
| if len(self.arrays) == 0: | ||
| arr = np.empty(self.shape, dtype=float) | ||
| return arr.transpose() if transpose else arr | ||
| farr = np.empty(self.shape, dtype=float) |
There was a problem hiding this comment.
i'm guessing the f represents float? Rather than guessing can you rename this, maybe "float_arr" or "empty" or "empty_arr"
while on the subject, I've never guessed or seen anything to give a clue as to what tipo is?
There was a problem hiding this comment.
while on the subject, I've never guessed or seen anything to give a clue as to what tipo is?
i think its DtypeObj; no idea on why the name was chosen
| @final | ||
| @property | ||
| def array(self): | ||
| def array(self) -> ArrayLike: |
There was a problem hiding this comment.
while here can you add error: "SingleDataManager" has no attribute "arrays"; maybe "array" comment above the ignore below.
| # _SupportsDtype, str, Tuple[Any, int], Tuple[Any, Union[int, | ||
| # Sequence[int]]], List[Any], _DtypeDict, Tuple[Any, Any]]" | ||
| result = np.empty(x.size, dtype=dtype) # type: ignore[arg-type] | ||
| # x and y are both ndarrays -> common_dtype is np.dtype |
There was a problem hiding this comment.
maybe we should overload find_common_type. could add a # TODO
| fill_value = 0 | ||
| # error: Non-overlapping equality check (left operand type: "dtype[Any]", right | ||
| # operand type: "Type[bool]") | ||
| elif dtype == bool: # type: ignore[comparison-overlap] |
There was a problem hiding this comment.
looks like a false positive (maybe from numpy types)
There was a problem hiding this comment.
np.bool_ does not inherit from bool: https://numpy.org/doc/stable/reference/arrays.scalars.html
There was a problem hiding this comment.
The error message is Non-overlapping equality check.... The numpy types should allow a check with a type. either bool, np.bool_ and str, object or any other type. Doing the equality check does not raise a type error, but should always return bool.
import numpy as np dtype = np.dtype("bool") print(dtype) print(dtype == bool) print(dtype == np.bool_) print(dtype == "bool") print(dtype == object) gives
bool True True True False In the numpy type annotations for 1.21.3, dtype.__eq__ is not defined, but looks like this is fixed on master. numpy/numpy#20184
There was a problem hiding this comment.
so what's the suggested action here?
There was a problem hiding this comment.
what you've done is fine. but we could also leave these ignores for now.
Once we are checking with the latest numpy types we could maybe revert all the changes that have been made to-date to get round this. wdyt?
| # no NaNs - can just concatenate | ||
| result = cat_safe(all_cols, sep) | ||
| | ||
| out: Index | Series |
There was a problem hiding this comment.
can you also add this as the return type of the function signature.
| result = Index( # type: ignore[assignment] | ||
| result, dtype=object, name=self._orig.name | ||
| ) | ||
| out = Index(result, dtype=object, name=self._orig.name) |
There was a problem hiding this comment.
maybe could just return here and no need for the variable type annotation.
pandas/io/stata.py Outdated
| # error: Non-overlapping equality check (left operand type: "dtype[Any]", right | ||
| # operand type: "Type[signedinteger[Any]]") | ||
| if dtype == np.int8: # type: ignore[comparison-overlap] | ||
| if dtype.type == np.int8: |
There was a problem hiding this comment.
does this still work with is?
There was a problem hiding this comment.
i think so, will check
| Thanks @jbrockmendel |
3 participants
