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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mAP calculation for areas with 0 predictions #1080

Merged
merged 3 commits into from Jun 9, 2022

Conversation

23pointsNorth
Copy link
Contributor

@23pointsNorth 23pointsNorth commented Jun 8, 2022

The issue was first dicussed here: #1061 (comment)

In cases of iou_type segm mAP calculation with 0 predictions, the standard box_area calculation is needed. This is fixed to use the updated compute_area method.

Sidenotes:

  • if gt_label_mask is with len>0, accessing a python list would fail -> need to casting
  • compute_area assumes a list of areas to be computed, thus the need to expand dims in cases a single element is selected from the array.

Has been tested to work with bbox and segm.

What does this PR do?

Fixes part of a bug first highlighted as in #1061.

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 馃檭

Would appreciate if this is added to the next patch release! Maybe I was too late for 0.9.1 :(

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

Can you add a test for this configuration? 馃惏

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #1080 (5a54a39) into master (9962764) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1080   +/-   ##
======================================
- Coverage      95%     95%   -0%     
======================================
  Files         181     181           
  Lines        8103    8103           
======================================
- Hits         7672    7660   -12     
- Misses        431     443   +12     

@23pointsNorth
Copy link
Contributor Author

23pointsNorth commented Jun 9, 2022

Hi @Borda ,

I am having issues running the test locally (make test assumes to have metrics/_data/detection/instance_segmentation_inputs.json, and those I assume are fetched separately not through e.g. make download-data).

I've added what I expect to be a sensible test (0 gt, 0 predictions), but hard to evaluate if it's the right spot or if it impacts some of the numerical test you have already set.

Or laternatively make a case like this with flipped gt/pred?

@Borda
Copy link
Member

Borda commented Jun 9, 2022

I am having issues running the test locally (make test assumes to have metrics/_data/detection/instance_segmentation_inputs.json, and those I assume are fetched separately not through e.g. make download-data).

good point we will add it to our make file...
the download link is https://pl-public-data.s3.amazonaws.com/metrics/data.zip

@Borda Borda mentioned this pull request Jun 9, 2022
4 tasks
@23pointsNorth
Copy link
Contributor Author

23pointsNorth commented Jun 9, 2022

Thanks! I assume also in .requirements/text.txt one needs to add tensorflow (because of bert -> huggingface_hub -> keras_mixin -> tf)

ERROR tests/text/test_bertscore.py - ModuleNotFoundError: No module named 'tensorflow'

Is there a way to run just detection tests?

edit: python3 -m pytest tests/detection should do it.
edit2: After the latest push, the above command makes the tests/detection/mean_ap.py finish.

@mergify mergify bot added the ready label Jun 9, 2022
@SkafteNicki SkafteNicki merged commit 84274ab into Lightning-AI:master Jun 9, 2022
Borda pushed a commit that referenced this pull request Jun 14, 2022
* Fix mAP calculation for areas with 0 predictions

The issue was first dicussed here: #1061 (comment)

* chlog

Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>

(cherry picked from commit 84274ab)
@23pointsNorth
Copy link
Contributor Author

Do you folks have any pointers to when the next patch version would be released?

@Borda
Copy link
Member

Borda commented Jun 22, 2022

Do you folks have any pointers to when the next patch version would be released?

we can do one next week :)

@dcela
Copy link

dcela commented Jun 28, 2022

Ran into the same issue, for now will build from current master!

@dcela
Copy link

dcela commented Jun 28, 2022

If there is interest in getting people just to improve documentation, I would be willing to help!

@Borda
Copy link
Member

Borda commented Jun 29, 2022

If there is interest in getting people just to improve documentation, I would be willing to help!

please do 馃惏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants