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

Vec3D transition shortcomings hidden by mypy failures #211

Closed
supersergiy opened this issue Jan 5, 2023 · 5 comments
Closed

Vec3D transition shortcomings hidden by mypy failures #211

supersergiy opened this issue Jan 5, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@supersergiy
Copy link
Member

supersergiy commented Jan 5, 2023

@nkemnitz discovered several breaking issues introduced by the Vec3D change.
The most notable one is the breakage of the interpolate flow, addressed by #210, #209.

Several questions remain:

  1. _VecND is currently not compatible with the Sequence protocol. This requires annotations like Union[Sequence[float], VecND] even in places that rely only on __len__ and __getitem__. Is there a good solution to this? fix: _VecND inherits from abc.Sequence #213
    image
  2. How about vec.list()/vec.as_list()/vec.to_list()? Same for tuple
  3. Can we include to Vec3D in the [Docs]? It's challenging for the new users to know how to operate it.
  4. Most importantly, how come the typeguard error where tensor_ops.interpolate expected sequence but got Vec3D was not caught by mypy? The error is fixed by fix: VecND support for tensor_ops.interpolate #210 , but I expect mypy to be able to catch this.
@supersergiy supersergiy added the bug Something isn't working label Jan 5, 2023
@nkemnitz
Copy link
Collaborator

nkemnitz commented Jan 6, 2023

For runtime checks you can already do Sequence.register(_VecND). Doesn't help the static type checkers, though: python/mypy#2922

Nevermind - Just inheriting from abc.Sequence works. Tried the same with typing.Sequence yesterday and got a strange pylance error

@nkemnitz
Copy link
Collaborator

nkemnitz commented Jan 6, 2023

@supersergiy: Regarding 2) Is there an advantage of this over list(vec) or tuple(vec)?

@dodamih
Copy link
Collaborator

dodamih commented Jan 6, 2023

So 1) is (at least checking for typing bit, not the other things that need to be implemented) fixed by the PR linked - are the other mixin functions actually needed?

For 2), I think list(vec) or tuple(vec) is clearer - it works as is.

For 3), I will write docs for VecND.

For 4), I've looked into this a little and it looks like typing.Sequence and collections.abc.Sequence being different might have something to do with it. Still looking into it.

@supersergiy
Copy link
Member Author

supersergiy commented Jan 6, 2023

Problem with 4 is more serious than just Sequence: https://github.com/ZettaAI/zetta_utils/pull/217/files#diff-9a1f1394cc41ddf4e36389c4dd6a230a4cef5a09496e37eeb13a294e06776068R69

At the same time, reveal_type(flow_schema) gives:

interpolate_flow.py:61: note: Revealed type is 
"zetta_utils.mazepa_layer_processing.common.chunked_apply_flow.ChunkedApplyFlowSchema[
[src: torch._tensor.Tensor, 
scale_factor: Union[builtins.float, typing.Sequence[builtins.float]], 
mode: Union[Literal['nearest'], Literal['nearest-exact'], Literal['linear'], Literal['bilinear'], Literal['bicubic'], Literal['trilinear'], Literal['area'], Literal['img'], Literal['field'], Literal['mask'], Literal['segmentation']], 
mask_value_thr: builtins.float =], zetta_utils.layer.volumetric.index.VolumetricIndex, 
None]"

but reveal_type(flow_schema.__call__): Revealed type is "def (idx: IndexT?, *args: P.args?, **kwargs: P.kwargs?) -> zetta_utils.mazepa.flows.Flow"

@supersergiy
Copy link
Member Author

This used to work. I wonder if something broke our mypy plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants