Skip to content

simplify boundingbox kernels with jit compatible ellipsis #5861

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

Merged
merged 2 commits into from
Apr 22, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Apr 22, 2022

Addresses #5781 (comment). Cross posting my answer there:

Good catch. I was under the impression ... was not supported by torch.jit.script in general, but this seems fine:

@torch.jit.script
def foo(x: torch.Tensor) -> torch.Tensor:
    y = x.clone()
    y[..., 0::2] += 1
    return y

It seems, only the explicit indices are not supported:

@torch.jit.script
def bar(x: torch.Tensor) -> torch.Tensor:
    y = x.clone()
    y[..., [0, 2]] += 1
    return y
RuntimeError: 
Ellipses followed by tensor indexing is currently not supported:
[...]
def bar(x: torch.Tensor) -> torch.Tensor:
    y = x.clone()
    y[..., [0, 2]] += 1
    ~~~~~~~~~~~~~ <--- HERE
    return y

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM.

If you look into the codebase for .view(-1, 4) you will find multiple other occurrences (for example at horizontal_flip_bounding_box, vertical_flip_bounding_box, affine_bounding_box etc. Should we fix this as well?

@vfdev-5 thoughts?

@pmeier
Copy link
Collaborator Author

pmeier commented Apr 22, 2022

The examples you mentioned are all using explicit indices and thus cannot simplified as explained above:

bounding_box[:, [0, 2]] = image_size[1] - bounding_box[:, [2, 0]]

bounding_box[:, [1, 3]] = image_size[0] - bounding_box[:, [3, 1]]

points = bounding_box[:, [[0, 1], [2, 1], [2, 3], [0, 3]]].view(-1, 2)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 22, 2022

IMO, view should be very tiny op. Great that we could simplify the code for this case and I think it is not a big deal that we couldn't for other cases... Maybe later when JIT will support that.
I think we can merge this PR.

@datumbox datumbox merged commit c82b86d into pytorch:main Apr 22, 2022
@pmeier pmeier deleted the jit-ellipsis branch April 22, 2022 14:25
facebook-github-bot pushed a commit that referenced this pull request May 6, 2022
…5861)

Summary: Co-authored-by: Vasilis Vryniotis <[email protected]>

Reviewed By: jdsgomes, NicolasHug

Differential Revision: D36095711

fbshipit-source-id: 79ae27e68fe0bb631dce44cae9ad0f8a8e3145a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants