Adds torchscript Compatibility to box_convert #2737

oke-aditya · 2020-10-01T12:16:41Z

Follow up of #2710 .

I fixed the small naming issue as mentioned.
Added tests for JIT

vfdev-5 · 2020-10-01T12:21:11Z

test/test_ops.py

-    # def test_bbox_convert_jit(self):
-    #     box_tensor = torch.tensor([[0, 0, 100, 100], [0, 0, 0, 0],
-    #                               [10, 15, 30, 35], [23, 35, 93, 95]], dtype=torch.float)
+    def test_bbox_convert_jit(self):


I was wondering if it would be more simple to check scripted function in all your test_bbox_*:

scripted_fn = torch.jit.script(ops.box_convert) for fn in [ops.box_convert, scripted_fn]: # ...

Possible, this PR is open for a lot of refactoring 👍 .

oke-aditya · 2020-10-01T13:44:18Z

The error sounds really trivial, but I cannot understand why it shouldn't be able to script it?
From CircleCI build. I could reproduce this locally on my Windows PC as well.

vfdev-5 · 2020-10-01T13:56:00Z

Error says it is missing else branch, so it means not all clauses are well covered...

oke-aditya · 2020-10-01T14:00:21Z

Is there a need for else branch for JIT to work? I have simply written if elif and not written any else block in entire code reason being in future we can simply continue coding without trying to recall what else was.
Other Tests worked well so I guess there is no logical error in code. May I know what should be probable fix?
I'm not much experienced in such jit errors so sorry if this question is too dumb.

vfdev-5 · 2020-10-01T14:26:05Z

I think we have to recode box_convert such that it has more clean control flow. For example

def box_convert(boxes: Tensor, in_fmt: str, out_fmt: str) -> Tensor:

    if in_fmt == out_fmt:
        boxes_converted = boxes.clone()
        return boxes_converted
    
    if in_fmt == "xyxy":
        if out_fmt == "cxcywh":
            return _box_xyxy_to_cxcywh(boxes)
        else:
            raise ValueError("...")
    elif in_fmt == "xywh":
        if out_fmt == "xyxy":
            return _box_xywh_to_xyxy(boxes)
        else:
            raise ValueError("...")
    elif in_fmt == "cxcywh":
        if out_fmt == "xyxy":
            return _box_cxcywh_to_xyxy(boxes)
        else:
            raise ValueError("...")
    else:
        raise ValueError("...")

Any thoughts ?

PS: maybe we have to replace assert by if something raise exception

vision/torchvision/ops/boxes.py

Line 158 in 6cbdf27

assert in_fmt in allowed_fmts

oke-aditya · 2020-10-01T14:31:05Z

Let @fmassa have a look.
The above is OK though I preferred assert statement. Still, I don't understand why JIT must need else after if
Torchscript support is a priority, we can refactor it to get it to work (even if code becomes a bit clumsy)

vfdev-5 · 2020-10-01T14:34:21Z

Still, I don't understand why JIT must need else after if

Because some clauses do not define the output and it is not evident whether they couldn't happen in runtime.

oke-aditya · 2020-10-01T14:37:49Z

Oh now I get the point. IIRC JIT needs to know what will exactly happen to code in future so it needs all possibilities right?
That's why it can compile Just In Time before execution. 👍

…ipt_bbox

oke-aditya · 2020-10-05T03:22:59Z

I got the JIT test passing by simply adding else with ValueError. Now we can refactor the code.
Please let me know the suggestions. 👍
@fmassa @vfdev-5

codecov · 2020-10-05T03:27:54Z

Codecov Report

Merging #2737 into master will increase coverage by 0.32%.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##           master    #2737      +/-   ##
==========================================
+ Coverage   72.42%   72.75%   +0.32%     
==========================================
  Files          96       96              
  Lines        8313     8309       -4     
  Branches     1293     1292       -1     
==========================================
+ Hits         6021     6045      +24     
+ Misses       1903     1871      -32     
- Partials      389      393       +4

Impacted Files	Coverage Δ
torchvision/ops/boxes.py	`95.29% <82.35%> (+2.03%)`	⬆️
torchvision/ops/_box_convert.py	`100.00% <100.00%> (ø)`
torchvision/io/_video_opt.py	`39.37% <0.00%> (+16.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e639d3...3094f03. Read the comment docs.

vfdev-5

Thanks for the update.

vfdev-5 · 2020-10-05T08:56:49Z

torchvision/ops/boxes.py

@@ -167,25 +167,35 @@ def box_convert(boxes: Tensor, in_fmt: str, out_fmt: str) -> Tensor:
            boxes_xyxy = _box_xywh_to_xyxy(boxes)
            if out_fmt == "cxcywh":
                boxes_converted = _box_xyxy_to_cxcywh(boxes_xyxy)
+            else:
+                raise ValueError("Unsupported Boundig Box Conversions for given in_fmt and out_fmt")


It would be nice to add tests to improve the coverage.

There is assert statement before these ValueError codeblocks. So the test will never reach this stage. Should I remove the assert statements?

yes, please

fmassa

Thanks a lot for the work @oke-aditya !

TorchScript can be tricky at times, and many times we don't quite understand what's going on and why it doesn't want to accept our code.

Your PR looks ok to me (although I would prefer to keep the assertion checks in lines 157-159).
I've also committed two different refactoring proposals in https://github.com/fmassa/vision-1/tree/proposal_box_convert_torchscript (one in each commit), which I think could make the code reading easier and with less checks.

Let me know what you think!

fmassa · 2020-10-06T10:06:23Z

torchvision/ops/boxes.py

-    allowed_fmts = ("xyxy", "xywh", "cxcywh")
-    assert in_fmt in allowed_fmts
-    assert out_fmt in allowed_fmts


Is this not supported by torchscript?
I just tried the following and it worked:

In [1]: import torch In [2]: def f(x: str): ...: L = ('a', 'b') ...: assert x in L ...: return x ...: In [3]: ff = torch.jit.script(f) In [4]: print(ff.code) def f(x: str) -> str: if torch.__contains__(["a", "b"], x): pass else: ops.prim.RaiseException("AssertionError: ") return x

I asked @vfdev-5 and removed it. We removed it since then tests will not trigger value error as assert would stop this. I added tests to check ValueError too. I am not sure if I should add it back.

If assert is not good, then let's change instead to ValueError, something like

if in_fmt not in allowed_fmts: raise ValueError(...)

Having those checks is important for better user experience, otherwise their code could silently fail

fmassa · 2020-10-06T10:20:40Z

torchvision/ops/boxes.py

@@ -167,25 +164,35 @@ def box_convert(boxes: Tensor, in_fmt: str, out_fmt: str) -> Tensor:
            boxes_xyxy = _box_xywh_to_xyxy(boxes)
            if out_fmt == "cxcywh":
                boxes_converted = _box_xyxy_to_cxcywh(boxes_xyxy)
+            else:
+                raise ValueError("Unsupported Bounding Box Conversions for given in_fmt and out_fmt")


You don't actually need those to make torchscript happy. The problem is that boxes_converted is not defined in the other branch.
You can define it outside of the if so that torchscript knows its type.

oke-aditya · 2020-10-06T11:47:43Z

Both proposals look fine to me. I guess it is matter of coding style.
I find that propoal 1 is better.
I think we don't need boxes_converted or rather we can have inplace parameter if needed.
Let me try with torchscript for proposal1. If it works well we can keep that 👍

fmassa · 2020-10-06T11:51:40Z

@oke-aditya we don't need boxes_converted, and we don't need inplace flag either because we won't be modifying the tensor inplace.
Notice that doing

def f(a):
    a = a + 1
    return a

doesn't change the original a (it's out of place), while

def f(a):
    a += 1
    return a

performs it in-place.

The advantage of proposal 2 is that it's slightly simpler, as we don't need to have some torchscript-specific constructs like defining first the type of the variable, and having to assert it being not None to let torchscript know about its return type.

oke-aditya · 2020-10-06T11:57:08Z

Sounds great. Let me re-run with proposal2 with the ValueError tests as well. Once the tests pass. I will push it 👍

fmassa

Thanks a lot!

* fixies small bug in box_convert * activates jit test * Passes JIT test * fixes typo * adds error tests, removes assert * implements to proposal2

oke-aditya added 2 commits October 1, 2020 17:22

fixies small bug in box_convert

1dd9189

activates jit test

6cbdf27

vfdev-5 reviewed Oct 1, 2020

View reviewed changes

oke-aditya and others added 3 commits October 5, 2020 08:10

Merge branch 'master' into tscript_bbox

ff8b906

Passes JIT test

08a3567

Merge branch 'tscript_bbox' of github.com:oke-aditya/vision into tscr…

605306c

…ipt_bbox

vfdev-5 reviewed Oct 5, 2020

View reviewed changes

oke-aditya and others added 3 commits October 5, 2020 23:23

Merge branch 'master' into tscript_bbox

11ab169

fixes typo

169d1bc

adds error tests, removes assert

fd5c660

oke-aditya requested a review from vfdev-5 October 5, 2020 18:43

fmassa reviewed Oct 6, 2020

View reviewed changes

oke-aditya and others added 2 commits October 7, 2020 09:20

implements to proposal2

f8114c6

Merge branch 'master' into tscript_bbox

3094f03

oke-aditya requested a review from fmassa October 7, 2020 07:59

fmassa approved these changes Oct 7, 2020

View reviewed changes

fmassa merged commit 5320f74 into pytorch:master Oct 7, 2020

oke-aditya deleted the tscript_bbox branch October 7, 2020 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds torchscript Compatibility to box_convert #2737

Adds torchscript Compatibility to box_convert #2737

oke-aditya commented Oct 1, 2020

vfdev-5 Oct 1, 2020

oke-aditya Oct 1, 2020

oke-aditya commented Oct 1, 2020 •

edited

Loading

vfdev-5 commented Oct 1, 2020

oke-aditya commented Oct 1, 2020

vfdev-5 commented Oct 1, 2020

oke-aditya commented Oct 1, 2020 •

edited

Loading

vfdev-5 commented Oct 1, 2020

oke-aditya commented Oct 1, 2020

oke-aditya commented Oct 5, 2020 •

edited

Loading

codecov bot commented Oct 5, 2020 •

edited

Loading

vfdev-5 left a comment

vfdev-5 Oct 5, 2020

oke-aditya Oct 5, 2020

vfdev-5 Oct 5, 2020

oke-aditya Oct 5, 2020

fmassa left a comment

fmassa Oct 6, 2020

oke-aditya Oct 6, 2020 •

edited

Loading

fmassa Oct 6, 2020

fmassa Oct 6, 2020

oke-aditya commented Oct 6, 2020

fmassa commented Oct 6, 2020

oke-aditya commented Oct 6, 2020 •

edited

Loading

fmassa left a comment

Adds torchscript Compatibility to box_convert #2737

Adds torchscript Compatibility to box_convert #2737

Conversation

oke-aditya commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya commented Oct 1, 2020 • edited Loading

vfdev-5 commented Oct 1, 2020

oke-aditya commented Oct 1, 2020

vfdev-5 commented Oct 1, 2020

oke-aditya commented Oct 1, 2020 • edited Loading

vfdev-5 commented Oct 1, 2020

oke-aditya commented Oct 1, 2020

oke-aditya commented Oct 5, 2020 • edited Loading

codecov bot commented Oct 5, 2020 • edited Loading

Codecov Report

vfdev-5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya commented Oct 6, 2020

fmassa commented Oct 6, 2020

oke-aditya commented Oct 6, 2020 • edited Loading

fmassa left a comment

Choose a reason for hiding this comment

oke-aditya commented Oct 1, 2020 •

edited

Loading

oke-aditya commented Oct 1, 2020 •

edited

Loading

oke-aditya commented Oct 5, 2020 •

edited

Loading

codecov bot commented Oct 5, 2020 •

edited

Loading

oke-aditya Oct 6, 2020 •

edited

Loading

oke-aditya commented Oct 6, 2020 •

edited

Loading