-
Notifications
You must be signed in to change notification settings - Fork 407
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 support for ObjectDetection #758
Conversation
c6a6a87
to
ee680fe
Compare
So the tests are failing due to warnings caused by the new Multi-Weight API torchvision introduced in If we want to continue supporting the minimum torchvision 0.10.0, I could just ignore the warnings. Otherwise, I can update What do you guys think? |
This is great, thanks so much! For torchvision dependency version, see what I did in #631 for the other trainers. It's possible to check the version of torchvision and use different syntax for each version. It's kinda ugly and I don't like it, but I'm not sure if it's wise to just drop support for all older torchvision versions. Open to feedback on this though. For the torchmetrics dependency version, what was the reason for the version bump? It seems like MeanAveragePrecision was introduced in 0.7.0 (renamed from MAP) according to the changelog. |
Saw this snippet.
This can work. Since the number of backbones supported also include the Agree with not dropping support. We only need to drop a version if it breaks something or if it requires really ugly code to handle it. Think we're good for now.
You're right. Think it was just the filename that was changed ( |
ee680fe
to
6852b3b
Compare
Another option would be to change the |
Sure, that could work. That way the user would be able to specify the weights to be used and if they leave it unspecified, we can just use the default weights. But specifying weights is only possible for |
Hmm, but then it becomes difficult to store this configuration in a YAML file... |
Right. The YAML would only be informative if you're using torchvision > 0.12 |
But even if the YAML is torchvision > 0.12 specific, can you store objects in YAML or only strings/floats/bools? We could store a string and exec it but that feels like a security risk... |
The weights in the YAML can be string. We could just specify it like this. experiment:
task: "nasa_marine_debris"
name: "nasa_marine_debris_test"
module:
detection_model: "faster-rcnn"
backbone: "resnet50"
+ pretrained: "ResNet50_Weights.IMAGENET1K_V1"
num_classes: 2
learning_rate: 1.2e-4
learning_rate_schedule_patience: 6
verbose: false
datamodule:
root_dir: "data/nasamr/nasa_marine_debris"
batch_size: 4
num_workers: 56
val_split_pct: 0.2 The pretrained arg can then be passed into the |
Ah, we could use Let's keep it like this for now but strongly consider dropping support for older torchvision at some point. |
The |
Coverage drop is mainly caused by the code required for supporting minimum versions. On that note, has anybody looked into combining the coverage reports for normal tests and minimal tests via |
6852b3b
to
e31b665
Compare
We run tests on both the latest version of dependencies and the minimum version of dependencies. Both tests generate coverage reports, and should automatically be merged when uploaded to codecov. This works on all of our other PRs, not sure why coverage is dropping for you. |
e31b665
to
eeddbcd
Compare
This usually means that coverage has increased in main since this branch was created. Usually, rebase or merge commits solve this. Wish codecov was smarter about this... |
Since I've opened this PR, there have been a total of 4 commits merged to main. The two tutorial commits, one related to Sentinel2 and the torchvision constraint fix so it's unlikely a coverage increase is the reason. I wonder if this has to do with me rebasing and squashing commits in this branch. |
Yep, I have no idea what's going on here. It's just this PR, right? Probably best to reach out to codecov and see if this is a bug in codecov. I'm focusing on 0.3.1 stuff at the moment, but if this is still broken after 0.3.1 is finished I can try to report it to codecov myself. |
I went down the torchvision.detection rabbit hole awhile back. I think my only complaint with it was that they don't support negative sample images that don't have any objects in them during training. Do you know if this has changed? I need to catch up on all the releases. |
Also I believe they added support for constructing a resnet fpn backbone from a pretrained torchvision resnet model. @calebrob6 and @anthonymlortiz, this might be of interest because we could add support for pretrained SSL resnet backbones to this task. |
@isaaccorley does this need a predict step? |
be9e65d
to
a9953a0
Compare
Ideally yes, we can have a predict step that returns the predicted boxes and class labels per sample. |
Let's add that here before we forget, I want to make sure all of our trainers support the same things. |
@ashnair1 Let me know if you want some help on it. I can walk you through the predict_steps I made for the classifiers. |
* Filter out invalid boxes * Add label key to batch * Add plot function
a9953a0
to
074ec1e
Compare
Added |
* Prepare NasaMarineDebris dataset & datamodule * Filter out invalid boxes * Add label key to batch * Add plot function * Add detection task * Add tests * Fix conf arg * Add test for non pretrained backbones * Coverage for when datamodule has no plot fn * Separate out tests * self.forward(x) -> self(x) * Add predict_step * list -> List
Adds a new
ObjectDetectionTask
which uses torchvision'sFaster-RCNN
under the hood.Closes #442