-
Notifications
You must be signed in to change notification settings - Fork 7k
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
masks_to_bounding_boxes
op
#4290
Conversation
Hi @0x00b1! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Sorry for an early poke at this PR. I think it would be nice to place the test in test_ops.py I'm not sure of how it should be tested. My initially were we could manually create boolean masks and test them like the ones done in utils.draw_segmentation_masks test? Also the code can be kept directly in boxes ? No strong opinion here though. |
Hi, @oke-aditya! I appreciate any and all feedback! 😄 My code organization was my preference but I am more than happy to adopt whatever convention you or any other maintainers prefer. I will parametrize the |
Not sure if they fall into same category, but there too we hardcoded some of the boxes and masks for images to test them. My initial thought was that this function too would be tested in similar way instead of drawing random shapes / boxes on an image. P.S. I'm just a small contributor to the library (also a novice developer) so please don't mind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR @0x00b1 and @oke-aditya for the review. I made a few comments and will look at the rest once this isn't draft anymore. Thanks for the initiative of writing a gallery example!!
test/test_masks_to_bounding_boxes.py
Outdated
|
||
@pytest.fixture | ||
def masks() -> torch.Tensor: | ||
with PIL.Image.open(os.path.join(ASSETS_DIRECTORY, "masks.tiff")) as image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be possible to write a test without the need for new images and hard-coded coordinates?
Ideally, we could generate random masks and have a super simple version of masks_to_boxes
which we could use as the reference implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I wrote about this elsewhere in the thread. I'd love to add a generator for various outputs similar to the function @goldsborough and I wrote for scikit-image (skimage.draw.random_shapes
). However, would you mind if I did this in a follow-up commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NicolasHug a friendly bump
@0x00b1 Just checking that you still plan to complete the PR. Please let me know :) |
@datumbox Yep! I was on vacation last week (it was lovely) and started on-boarding at Facebook yesterday. I'll finish this today or tomorrow. Thanks, @NicolasHug for the comments! |
Can you generalize this to 3D images? |
@RylanSchaeffer it's definitely worth discussing it on a new issue. I would prefer if we did this on a separate PR to avoid blocking it for longer. |
@datumbox I have two opinions. On one hand, I agree that not blocking this PR is good. On the other, a half solution means a fix to the complete problem will probably be delayed. |
This PR isn't half a solution, it's a complete solution to a complete problem: 2D images. 3D images are a different problem which we'll be happy to tackle at a future time once this PR is merged, as @datumbox suggested |
That's a strange way to think about things. Imagine someone submitted a cross entropy loss implementation for a 2D array. By your metric, it's a complete solution to a complete problem i.e. 1-dimensional classification. But look at the cross entropy loss implementation: it works for arbitrary dimensions, not just one, because we shouldn't be limited to a 2D array. On this topic, the real problem is more general than 2D images. For us, the real problem is: given a N-dimensional segmentation mask, how to convert the mask to N-dimensional bounding boxes? A solution for 2D is a partial solution. |
@RylanSchaeffer , just because we can generalize a problem doesn't mean that one problem is less "real" or "complete" than the other. 2D masks are a normal use-case that a lot of people have and solving this will be valuable on its own. When it comes to software development, a merged PR that solves one problem is worth more than an unmerged PR that solves 2 problems. Again, we'll be happy to consider an extension if the use-case is compelling. |
Ok issue opened! #4339 |
@RylanSchaeffer In addition to what Nicolas said, and for full transparency, here are some reasons for why we often choose not to go straight for the most generic/complicated implementation:
I think it's worth continuing the discussion of how this can be made generic on the new issue that you opened. @0x00b1 Welcome back. Sounds great, if you have any issues with the CI let us know and we can help. |
@RylanSchaeffer I agree that this should come in a future PR. But don't worry, 3D or n-D images are something I care about too so I'm more than happy to do that work. |
Co-authored-by: Aditya Oke <[email protected]>
Co-authored-by: Aditya Oke <[email protected]>
Co-authored-by: Aditya Oke <[email protected]>
Repurposing annotations | ||
======================= | ||
|
||
The following example illustrates the operations available in :ref:`the torchvision.ops module <ops>` for repurposing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some debugging I found out the reason for build_docs
CI failure. The problem is torchvision.ops
does not have a nice index on right side (basically a html link to #ops like transforms has). This causes CI failure.
We need to remove the ref, and it will work fine. This is slightly hacky fix, but works fine.
I tried running it locally. I could build the gallery example. It looks nice.
The following example illustrates the operations available in :ref:`the torchvision.ops module <ops>` for repurposing | |
The following example illustrates the operations available in the torchvision.ops module for repurposing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I appreciate the debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Allen you need to add docs to docs/ops.rst
where you can use.
.. autofunction:: masks_to_boxes
This will add docs for this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0x00b1 sorry for the back and forth. Adding an operator is possibly one of the most complex things as one needs to add many things across many files. I think we are almost there to merge. Let me summarize the comments that I think remain unresolved:
- Address the docs failure as described here: https://github.com/pytorch/vision/pull/4290/files#r712457642
- We missed one use of numpy vs torch. Just copy paste what you got on the examples and we should be good to go. https://github.com/pytorch/vision/pull/4290/files#r712884767
- Add the masks_to_boxes in docs as described here:
masks_to_bounding_boxes
op #4290 (review)
Nice catch! Fixed. |
It's no problem dude! I sincerely appreciate your and @oke-aditya's patience! In the future, it might be worth investigating whether someone should add a cookiecutter or cookiecutter-like method for generating op scaffolding. |
@datumbox OK. Everything has been addressed. Hopefully we don't see any CI failures! I would also be more than happy to squash these commits down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot @0x00b1. Congrats on your first contribution. :)
Hi @0x00b1 , Thank you for the great work on this PR! I only have 2 remaining comments at this point:
Would you or @oke-aditya be interested in a follow-up PR with these? The first point might be a bit trickier, but the second one should be reasonably simple. We can do them in separate PRs. Thanks! |
I'm fine with either. I will leave choice to @0x00b1 |
Also another thought about the gallery example. Adding this example will help users to convert PenFudan / Panopatic datasets easily to detection.
|
|
||
n = masks.shape[0] | ||
|
||
bounding_boxes = torch.zeros((n, 4), device=masks.device, dtype=torch.int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial thought was dtype should be torch.float
. Since all other ops follow float dtype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, also the above zeros needs to have a device:
torch.zeros((0, 4), device=masks.device)
Could you please send a PR that fixes these 2 issues? The rest of the doc/test improvements discussed here can happen on a separate PR.
Summary: * ops.masks_to_bounding_boxes * test fixtures * unit test * ignore lint e201 and e202 for in-lined matrix * ignore e121 and e241 linting rules for in-lined matrix * draft gallery example text * removed type annotations from pytest fixtures * inlined fixture * renamed masks_to_bounding_boxes to masks_to_boxes * reformat inline array * import cleanup * moved masks_to_boxes into boxes module * docstring cleanup * updated docstring * fix formatting issue * gallery example * use torch * use torch * use torch * use torch * updated docs and test * cleanup * updated import * use torch * Update gallery/plot_repurposing_annotations.py * Update gallery/plot_repurposing_annotations.py * Update gallery/plot_repurposing_annotations.py * Autodoc * use torch instead of numpy in tests * fix build_docs failure * Closing quotes. Reviewed By: datumbox Differential Revision: D31268025 fbshipit-source-id: 65f88779516ff0a411600a25b783f00369d56719 Co-authored-by: Aditya Oke <[email protected]> Co-authored-by: Aditya Oke <[email protected]> Co-authored-by: Aditya Oke <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]> Co-authored-by: Aditya Oke <[email protected]>
This (draft) pull request resolves #3960. I created a draft to kickoff new contributor on-boarding (e.g. CLA).
I'm working on a gallery example now. I'll also add test against different
dtypes
.