Skip to content

Conversation

msokoloff1
Copy link
Contributor

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Comment on lines +50 to +53
segmentation.extend([
box.start.x, box.start.y, box.end.x, box.start.y, box.end.x,
box.end.y, box.start.x, box.end.y
])
Copy link
Contributor

Choose a reason for hiding this comment

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

q: does the orientation matter for coco?

def rle_to_common(class_annotations, class_name):
mask = rle_decoding(class_annotations.segmentation.counts,
*class_annotations.segmentation.size[::-1])
return ObjectAnnotation(name=class_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not I was going to leave a comment "I don't see name in ObjectAnnotation but then realized that BaseAnnotation inherits from FeatureSchema. Imho it's quite confusing, it should be a composition rather than inheritance.

futures = []
coco_categories = {}

with ProcessPoolExecutor(max_workers=8) as exc:
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make executor an optional argument of from_common

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See question below for same comment on panoptic dataset.

coco_categories = {}
coco_things = {}
images = []
with ProcessPoolExecutor(max_workers=8) as exc:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about executor as arg. Not being able to specify concurrency would be pretty annoying to me as user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if I just let the number of workers be an arg? My thinking is that most users are going to want the same amount of parallelism - the max that the backend can handle. Also this script is unlikely to run within the context of another executor - users are going to either convert an entire dataset or not. So asking users to pass in the execturor is an extra step that will always be necessary.

@msokoloff1
Copy link
Contributor Author

I changed everything to use Pathlib - including the public interfaces. I am a little bit hesitant to keep it that way because we don't support that in other interfaces. Let's chat about this.

@msokoloff1 msokoloff1 merged commit a0d3b67 into develop Sep 15, 2021
msokoloff1 added a commit that referenced this pull request Feb 6, 2022
@msokoloff1 msokoloff1 deleted the ms/coco branch April 11, 2022 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants