TYP: require Index objects earlier in internals #33100
Conversation
| def create_block_manager_from_arrays( | ||
| arrays, names: Index, axes: List[Index] | ||
| ) -> BlockManager: | ||
| assert isinstance(names, Index) |
There was a problem hiding this comment.
In theory mypy will flag violations here already for us, so superfluous to have these here
There was a problem hiding this comment.
@simonjayhawkins is this correct? in a previous pass I found there were annotation-defying types being passed in this function that mypy had not caught
There was a problem hiding this comment.
yes, @WillAyd is correct, but in practice we probably need the run-time asserts to be sure. probably better to fail than potentially produce incorrect results.
in a previous pass I found there were annotation-defying types being passed in this function that mypy had not caught
if a variable is Any or the callsite is not being checked, then we can pass the wrong type.
| if passed == implied and e is not None: | ||
| raise e | ||
| return e |
There was a problem hiding this comment.
Rather than do this I think would be better to just mark this function with typing.NoReturn as the return value
There was a problem hiding this comment.
i recall that fails on our min version build.
There was a problem hiding this comment.
elsewhere (in particular in indexes.base) we've moved towards this pattern
pandas/core/internals/managers.py
Outdated
| # put "leftover" items in float bucket, where else? | ||
| # generalize? | ||
| items_dict = defaultdict(list) | ||
| items_dict: Dict[str, List] = defaultdict(list) |
There was a problem hiding this comment.
Was this throwing an error? I don't think this is required but if so should type using typing.DefaultDict
| @@ -1738,7 +1745,7 @@ def form_blocks(arrays, names, axes): | |||
| block_type = get_block_type(v) | |||
| items_dict[block_type.__name__].append((i, k, v)) | |||
|
|
|||
| blocks = [] | |||
| blocks: List[Block] = [] | |||
There was a problem hiding this comment.
Just as FYI but newer versions of mypy technically have better inferencing capabilities for empty sequence assignment like this. @simonjayhawkins
There was a problem hiding this comment.
id be happy to remove this; is that the suggested course of action?
There was a problem hiding this comment.
Thanks @jbrockmendel generally lgtm ex @WillAyd comments. A couple of suggestions but not blockers. The asserts are primarily for runtime debugging and not mypy?
| @@ -74,6 +77,8 @@ def arrays_to_mgr(arrays, arr_names, index, columns, dtype=None, verify_integrit | |||
| # from BlockManager perspective | |||
| axes = [columns, index] | |||
|
|
|||
| assert isinstance(columns, Index), type(columns) | |||
| assert isinstance(index, Index), type(index) | |||
There was a problem hiding this comment.
ensure_index on L70 and L75 should return an Index. can you type ensure_index instead of adding asserts
| @@ -209,6 +215,8 @@ def init_ndarray(values, index, columns, dtype=None, copy=False): | |||
| else: | |||
| block_values = [values] | |||
|
|
|||
| assert isinstance(columns, Index), type(columns) | |||
| assert isinstance(index, Index), type(index) | |||
There was a problem hiding this comment.
similar comment as above, can we type _get_axes instead of adding asserts.
| if passed == implied and e is not None: | ||
| raise e | ||
| return e |
There was a problem hiding this comment.
i recall that fails on our min version build.
|
i think requests have been addressed |
|
Thanks @jbrockmendel |
No description provided.