-
Notifications
You must be signed in to change notification settings - Fork 24
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
MobileNetv2 #26
MobileNetv2 #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution!
Overall, the implementation looks good. I have a couple of comments, mainly related to the fact that the pytorch implementation was followed a bit too closely 😄
If you have any questions for loading the pre-trained weights feel free to ping me on discord! Otherwise I can take care of that afterwards, no worries.
0bf0503
to
b116b54
Compare
- Remove configurable normalization (batchnorm was always used anyway) - Fix Conv2dNormActivation padding - Fix batchnorm rank - Fix classifier input and output ranks - Move utils to a simple closure for channel computation - Add PyTorchFileRecorder with remapping - Add structs documentation - Make residual settings constants - Remove dead comments
- Add inference example with image sample - Add README - Add NOTICES - Rename to mobilenetv2
@Arjun31415 I've made some changes to support the pre-trained weights and added an example which loads the weights for inference. Fixed some bugs that I uncovered when trying to load the weights, but otherwise mainly the form (missing README, NOTICES, added structs doc). I didn't put the IMAGENET1K_V1 weights since the official weights at this url cannot be loaded via candle-core (zip-rs issue: "invalid Zip archive: Could not find central directory end"). @antimora @nathanielsimard since I contributed, do any of you mind reviewing this as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good job, only a few minor comments.
@@ -0,0 +1,1034 @@ | |||
use burn::tensor::{backend::Backend, Device, Tensor}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laggui do we have the same file elsewhere? I feel like we should put this in another module if it's used in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in resnet and now in mobilenet. I too feel that we could classify the models say into image classification module and then have common utils, then have the models themselves be sub modules in it. It would also be helpful in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's used in both ResNet and MobileNet.. but right now the models are released separately. We should probably finish the discussion in #16 for loading weights since this is where the common code lies.
Not sure what the best approach would be.
@Arjun31415 just merged 🎉 Thanks for your contribution! |
time for mobilenetv3 :D Or fasterRCNN |
Work in progress. Model structure seems to be correct, could be simplified more by getting rid of a few enums.
Need help in loading pytorch weight 🥲