Rewrite test and fix masks_to_boxes implementation #4469
Conversation
test/test_ops.py
Outdated
|
|
||
| for dtype in [torch.int16, torch.int32, torch.int64]: | ||
| masks = torch.zeros((image.n_frames, image.height, image.width), dtype=dtype) | ||
| masks = _create_masks(image, masks) |
There was a problem hiding this comment.
One we know to create masks better way, without the image we can fix this,
There was a problem hiding this comment.
@oke-aditya Thanks for the PR. LGTM.
On the future, we could potentially do a couple of iterations to improve the tests of this operator. We should also look into the fact that the gallery uses an asset from tests, which is not great.
There was a problem hiding this comment.
Thanks @oke-aditya , some minor comments but LGTM
I agree with @datumbox 's box comment about not relying on the test directory in the galleries
|
I think I should have addressed all the issues 😄 Thanks for the detailed review |
|
I think I will followup to make gallery example a bit more robust. Also see if we can do with a PNG image and PNG mask (like in PenFudan) instead of a tiff. And a small demonstration to convert a segmentation dataset to detection. |
|
Thanks @oke-aditya ! Looking forward to the follow-ups. @datumbox should we remove the "bug" label for this PR and just use the same labels as for #4290? If we keep "bug" there's a high change this will end up as an entry in the bugfix section in the changlog, and this won't be accurate since this is something that hadn't been released yet. |
|
@NicolasHug Hmm... TLDR: Sure thing, go ahead. The slightly longer response is that the label is accurate in a sense that this PR fixes a bug. I do agree with you that it shouldn't be reported on the changelog as we are not fixing a bug of the previous release. So I think there might be an issue with how we use the tags as there are conflicting needs for example release notes require tagging fixes on old things as bug, while tracking better engineering initiatives requires tagging all. I think that's a good topic for DAPI or Vision weekly as we are going to bump on it again and again. |
|
I'll remove it for this one, but I agree this is something we could discuss and improve |
Summary: Co-authored-by: Nicolas Hug <[email protected]> Reviewed By: datumbox Differential Revision: D31268032 fbshipit-source-id: 61fe5ca1d28071d9683da519ea4f79a0bc417f48
Co-authored-by: Nicolas Hug <[email protected]> [ghstack-poisoned]
Co-authored-by: Nicolas Hug <[email protected]>
Fix the comments suggested in #4290 (comment)