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

Change coco utils to use high-level interface #105

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

schencej
Copy link
Contributor

Highlights:

  • gen_coco_sal() is gone and in its place is parse_coco_dset() which takes a kwcoco.CocoDataset object and creates matrices for use with the new high-level detector interface
  • sal-on-coco-dets was updated to use this new utility function and high-level interface combination
  • Updated testing of both modules
  • I added new logic to mock failed import of the extra dependencies even when they are installed
  • Updated the serialized detections example as well

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #105 (9e79338) into master (3d8122b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9e79338 differs from pull request most recent head d7088ba. Consider uploading reports for the commit d7088ba to get more accurate results

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
- Coverage   99.95%   99.95%   -0.01%     
==========================================
  Files          51       51              
  Lines        2144     2063      -81     
==========================================
- Hits         2143     2062      -81     
  Misses          1        1              
Flag Coverage Δ
unittests 99.95% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
xaitk_saliency/exceptions.py 100.00% <ø> (ø)
tests/utils/test_coco.py 100.00% <100.00%> (ø)
tests/utils/test_sal_on_coco_dets.py 100.00% <100.00%> (ø)
xaitk_saliency/utils/bin/sal_on_coco_dets.py 100.00% <100.00%> (ø)
xaitk_saliency/utils/coco.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d8122b...d7088ba. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Mar 23, 2022

This pull request introduces 1 alert when merging 697157b into bc1a71d - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Mar 23, 2022

This pull request introduces 1 alert when merging b9544c8 into bc1a71d - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@schencej
Copy link
Contributor Author

Using a fill with the CLI tool is now done by specifying it in the configuration for the GenerateObjectDetectorBlackboxSliency implementation. The fill currently isn't accounted for in the configuration suite of the existing implementations. I will address this in #97 since I changed the format of drise.py there.

@schencej
Copy link
Contributor Author

I believe the continue statements showing as not being covered by testing are actually covered. This seems to be related to some CPython optimization stuff, see this issue I found.

Copy link
Member

@Purg Purg left a comment

Choose a reason for hiding this comment

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

We can probably rename the gen_coco_sal module to something simpler now, like maybe just coco.

tests/utils/test_gen_coco_sal.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@Purg
Copy link
Member

Purg commented Mar 28, 2022

@schencej With the function name change to parse_coco_dset, it looks like the failing notebook needs an update to use that new name.

@schencej schencej force-pushed the update-coco-tools branch 3 times, most recently from a3f2ed7 to 9e79338 Compare March 28, 2022 19:57
Copy link
Member

@Purg Purg left a comment

Choose a reason for hiding this comment

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

LGTM. Just needs an autosquash and I think we're good to go. @brianhhu any last comments?

Copy link
Member

@brianhhu brianhhu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. I made a few very minor comments, none of them are really blocking this PR though.

examples/SerializedDetectionSaliency.ipynb Outdated Show resolved Hide resolved
examples/SerializedDetectionSaliency.ipynb Outdated Show resolved Hide resolved
xaitk_saliency/utils/bin/sal_on_coco_dets.py Outdated Show resolved Hide resolved
"\n",
"The number of masks for our image perturber is set to `20`.\n",
"The number of perturbation masks image is set to `20`.\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"The number of perturbation masks image is set to `20`.\n",
"The number of perturbation masks is set to `20`.\n",

@sonarcloud
Copy link

sonarcloud bot commented Mar 30, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@schencej
Copy link
Contributor Author

Addressed the last round of comments from @brianhhu and rebased. Should be good to go.

@brianhhu
Copy link
Member

LGTM, thanks- feel free to merge!

@Purg Purg merged commit fd2a8de into XAITK:master Mar 31, 2022
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.

None yet

3 participants