Mmdet adapter #1545
Merged
Mmdet adapter #1545
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A-Jacobson
commented
Sep 23, 2022
Landanjs
reviewed
Sep 26, 2022
There was a problem hiding this comment.
I think the code itself looks good although I'm not very familiar with using COCOEval and MMdetection, so I might have missed something.
The only obvious issues are with the docstrings. I commented on things that I thought were against our "standard" (although I may be mistaken and we may not have a standard in some cases...). Also, there are a few instances where lists (e.g. for COCO dictionary keys) are not rendered properly on the docs website although I'm not sure the format to get these to render properly.
Couple of questions:
- Would it be possible to test the MAP torchmetrics? I can imagine this be very difficult, but if it ever gets messed with in the future, it would be nice to be able to tell if it broke. A simple test like checking identical outputs results in a mAP of 1 may even be helpful.
- Are there references for the model output for both training and testing? Also, a definition for the mmedetection batch (is the test definition sufficient?)? If you think it would be appropriate to add these details to the code, I think it would make it a lot easier to use. If not, let's at least record somewhere
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Landanjs
approved these changes
Oct 6, 2022
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Co-authored-by: Landan Seguin <landan@mosaicml.com>
Co-authored-by: Landan Seguin <landan@mosaicml.com>
hanlint
approved these changes
Oct 11, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
mmdet adapter, tested with yolox and faster-rcnn.
MAP metric, taken from torchmetrics 0.6.0 (last working version)
allowed dict literals in our pre-hook config to accommodate mmdet configs.
maskrcnn/instance segmentation works for some models but needs more testing. Decided not to mention it in these docs as I'm not sure if all/most models work.