This repository has been archived by the owner on Oct 31, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
support RLE and binary mask #150
support RLE and binary mask #150
Changes from all commits
7df2301
d4226ad
fbadd1c
b91dd11
febb542
7f22baa
698010f
b8c5bca
e3e0a53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think the
mode
field for the Polygon is completely irrelevant.It is never used, but causes:
Question:
Wouldn't it be more consistent if the
convert
method of a Polygon would be renamed toconvert_to_mask
and would return a Mask instance?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.
Hi,
I agree with your points.
The reason why this is currently the case was that I wanted to keep the same interface between
Polygons
andBox
(which is not implemented, but is the single-box equivalent ofBoxList
).And my original idea was that we would be able to specify what was the underlying type of the data via the mode: is it a polygon, or a mask?
I'm not sure about changing the
convert
name of the method though.In general, I think both
box_list
andsegmentation_mask
could benefit from some better design / cleanup, but I'm not sure what that would beThere 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.
I think I cannot follow this part:
As in the current implementation, a
Polygon
instance:list
of polygonsPolygon
instance (which is referenced now, but should be hard-copied IMO)Mask
, which feature could be added if necessary (I am doing this to convert GTA binary masks to COCO Polygon format, but only because the binary masks are not supported).So the underlying data would be specified: Polygon.
On the other hand, about the
convert
function:convert
function takes an argument for the target mode, but actually it accepts just a single answer, which is odd.Polygon
can be onlyconvert
-ed to aMask
thanconvert
name is OK, but relies on the assumption that the data can be represented either inPolygon
s orMask
s and nothing else, which is not necessary a trivial assumption, so changing the name toconvert_to_mask
would be clear from the very first encounter.convert
orconvert_to_polygon
method to theMask
class as wellThere 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.
Those are all reasonable points, and I'm willing to accept PRs that improve the overall consistency and software design of the codebase
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.
@fmassa Thanks, these points were considered for the refactored version, PR #473