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

Add high-level interface and impls for blackbox detectors #100

Merged
merged 7 commits into from
Mar 21, 2022

Conversation

schencej
Copy link
Contributor

@schencej schencej commented Mar 9, 2022

No description provided.

@codecov
Copy link

codecov bot commented Mar 9, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (4949265) to head (d48708f).
Report is 143 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #100    +/-   ##
========================================
  Coverage   99.94%   99.95%            
========================================
  Files          44       50     +6     
  Lines        1757     2083   +326     
========================================
+ Hits         1756     2082   +326     
  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.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@schencej
Copy link
Contributor Author

I've realized that the we accept reference objectness scores as an input to generate which assumes that your model returns such a score. But the DetectImageObjects interface has no option to return an objectness score, so the perturbed detections never include it. Thoughts @Purg ?

@brianhhu
Copy link
Member

brianhhu commented Mar 15, 2022

Overall, LGTM- thanks @Purg for the detailed comments. Some minor suggestions:

  1. We should probably update the saliency algorithms table in the intro to point to this new implementation (vs. just DRISEScoring).

  2. We had a note in the DRISEScoring implementation about how to use DRISE with a single class prediction and confidence score. I think it might also make sense to either move or copy that docstring here to this implementation too.

  3. We haven't updated the DRISE.ipynb notebook as part of this PR- would it make sense to switch to this implementation here and remove some of the current boilerplate code in that notebook?

  4. Finally, now we have two potential DRISE-based tools, gen_coco_sal and DRISEStack. Can we make it somewhat more clear when one method is preferred over the other (e.g. if you have input detections, you should use gen_coco_sal)? Relatedly, there are no autodocs for the gen_coco_sal tool in ReadTheDocs, which might be useful to help distinguish these two implementations.

@Purg
Copy link
Member

Purg commented Mar 15, 2022

  1. We haven't updated the DRISE.ipynb notebook as part of this PR ...

That's not a bad idea. I believe an equivalent change was done when we updated the classifier black box interface.
We're pretty low on task parallelism right now, so this change being part of this PR seems fine to me.

  1. Two tools

My vision in this regard is that the gen_coco_sal is for the CLI interface. I believe @schencej and I have chatted about this before, but I'm happy to talk about it if desired. The majority of the gen_coco_sal is just the xaitk_saliency/impls/gen_object_detector_blackbox_sal/occlusion_based.py's PerturbationOcclusion._generate() method, but using kwcoco semantics for input detection extraction + formatting. We discussed last that with the "high-level" interface and implementations, those become the thing to use when programming, and the gen_coco_sal stuff is effectively "CLI only." Of course, given that, a chunk of, if not all of, the contents of that util could be shifted into the tool's script (xaitk_saliency/utils/bin/sal_on_coco_dets.py).

A ramification of that is that we will need to update the examples/SerializedDetectionSaliency.ipynb notebook to use the GenerateObjectDetectorBlackboxSaliency interface/implementation as it is taking the programmatic approach (as opposed to the CLI).

@brianhhu
Copy link
Member

Yeah, maybe I wasn't very clear. I wasn't necessarily proposing transitioning gen_coco_sal to use the new implementation in this PR (even though there is some shared code). I was mainly thinking about adding additional documentation (maybe in ReadTheDocs) that would help a new user decide whether to use the CLI tool or the programmatic interface. I imagine there could potentially be some confusion since a user would have to dive into the docstrings of each function to understand the differences and use cases.

@Purg
Copy link
Member

Purg commented Mar 15, 2022

Yeah, maybe I wasn't very clear. I wasn't necessarily proposing transitioning gen_coco_sal to use the new implementation in this PR (even though there is some shared code). I was mainly thinking about adding additional documentation (maybe in ReadTheDocs) that would help a new user decide whether to use the CLI tool or the programmatic interface. I imagine there could potentially be some confusion since a user would have to dive into the docstrings of each function to understand the differences and use cases.

That's fair. Part of what I was trying to say above is that the gen_coco_sal function as it is could likely just "go away", as the remaining details of it are, I think, CLI specific and could be rolled right into the CLI tool. As a fallout of that, none of the library methods exposed would use kwcoco objects/containers, which I currently think is OK (but may be convinced otherwise).

@schencej schencej force-pushed the high-level-drise branch 4 times, most recently from 54144d0 to 3a58f47 Compare March 16, 2022 14:13
@brianhhu
Copy link
Member

Thanks, overall this LGTM. Looks like there are minor differences in the final saliency map value ranges for the new high-level interface and the previous DRISE notebook (on master), although the rough shape of the maps look similar (maybe this is due to the detections being selected?) Also, there is a code smell for cognitive complexity, but I assume that's OK.

@schencej
Copy link
Contributor Author

Looks like there are minor differences in the final saliency map value ranges for the new high-level interface and the previous DRISE notebook (on master), although the rough shape of the maps look similar (maybe this is due to the detections being selected?)

I think this is due to the original detector model returning all 90 class probabilities while the SMQTK-Detection version returns 80 (throws out the 'N/A' class scores).

@schencej
Copy link
Contributor Author

schencej commented Mar 17, 2022

Edited the dets_to_formatted_mat function to get rid of the code smell and autosquahed. Should be good to go.

examples/DRISE.ipynb Outdated Show resolved Hide resolved
examples/DRISE.ipynb Outdated Show resolved Hide resolved
examples/DRISE.ipynb Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Mar 17, 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
2.1% 2.1% Duplication

@Purg Purg merged commit bc1a71d into XAITK:master Mar 21, 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.

3 participants