Skip to content
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

[issue-799] coco split into train/test; updated coco dataset module api #805

Merged
merged 12 commits into from Feb 8, 2022

Conversation

yromanyshyn
Copy link
Contributor

resolves #799

@yromanyshyn yromanyshyn added the refactoring Making significant changes to structure of code label Feb 2, 2022
@yromanyshyn yromanyshyn self-assigned this Feb 2, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@yromanyshyn
Copy link
Contributor Author

yromanyshyn commented Feb 2, 2022

update:

@ItayGabbay @nirhutnik
please take a look at the deepchecks/vision/metrics_utils/detection_precision_recall.py
I had to make a small fix to it, not sure if it is correct
The reason why I added it is because it was failing at that point

In general _compute_ap_recall was returning a dict with all values set to None, but
tensor cannot be created with none values, you will get an error if you try

RuntimeError: Could not infer dtype of NoneType

@nirhutnik
Copy link
Contributor

Seems good to me, but would prefer Gabbay take a look as its his code. If I understand correctly, this means that sometimes we get average precision = None (why?), and in those cases you want it to count as 0. Not sure what is done later with that information - if its summed up, then ok, but I guess it is averaged again and in that case maybe should be just ignored and not counted in the denominator as well.

But didn't dive into the code yet.

@yromanyshyn
Copy link
Contributor Author

also, are those two lines within VisionContext correct?
(that is a reason why notebook CI/CD check fails)

80: if train and test:
81:      train.validate_shared_label(test)

if I got it right, in object-detection task label is a 2d array each row of which
represent a separate object (bbox), and each image(label) could contain different
number of objects

we probably should only verify that number of columns is the same?

@nirhutnik
Copy link
Contributor

@yromanyshyn I think that Gabbay's PR solves that anyway as he transfers label validation to the LabelEncoder classes

pin_memory: bool = True,
object_type: Literal['Dataset', 'DataLoader'] = 'DataLoader'
) -> t.Union[DataLoader, vision.VisionDataset]:
"""Get the COCO dataset and return a dataloader.
Copy link
Member

Choose a reason for hiding this comment

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

I would call it the COCO 128 dataset, as the COCO dataset is much larger than that.

Copy link
Member

Choose a reason for hiding this comment

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

Also, did we validate the license of this dataset? and model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed,
it is GNU Version 3

makefile Outdated
xargs -P4 -I'{}' $(JUPYTER) nbconvert --execute '{}' \
--to notebook --stdout > /dev/null

$(JUPYTER) nbconvert --execute $$(find ./docs/source/examples -name "*.ipynb") --to notebook --stdout > /dev/null
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird but I initially thought that this was causing 'notebook check' failure. I have undone it

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you are correct. I'm now checking this

@@ -79,7 +79,10 @@ def compute(self):
**self._compute_ap_recall(ev["scores"], ev["matched"], ev["NP"])
}
if self.return_ap_only:
res = torch.tensor([res[k]["AP"] for k in sorted(res.keys())])
res = torch.tensor([
Copy link
Member

Choose a reason for hiding this comment

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

I think that's OK. This may happen in the case the model detected a class that doesn't exist in the test set.
Anyway, we are planning to replace this module in the near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@yromanyshyn yromanyshyn merged commit d546f38 into main Feb 8, 2022
@delete-merged-branch delete-merged-branch bot deleted the issue-799 branch February 8, 2022 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Making significant changes to structure of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT][CV] Create train-test COCO object detection dataset
3 participants