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.

Sign up for GitHub

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

Jump to bottom

Added GoogleNet (Inception V1) Model #47

Merged
merged 5 commits into from
Sep 4, 2023
Merged

Added GoogleNet (Inception V1) Model #47

merged 5 commits into from
Sep 4, 2023

Conversation

Sarvesh-Kesharwani
Copy link
Contributor

Corresponding to: #38

@Sarvesh-Kesharwani
Copy link
Contributor Author

chrome_tocLYwuZ9V

@Sarvesh-Kesharwani
Copy link
Contributor Author

chrome_CFz5HWx7ty

@Sarvesh-Kesharwani Sarvesh-Kesharwani changed the title GoogleNet (Inception V1) Model Implementation GoogleNet (Inception V1) Model Jul 24, 2023
@Sarvesh-Kesharwani Sarvesh-Kesharwani changed the title GoogleNet (Inception V1) Model Added GoogleNet (Inception V1) Model Jul 24, 2023
@Sarvesh-Kesharwani
Copy link
Contributor Author

@juliagsy I have implemented the model, it's test function, and I have also applied the General Pipeline guide, please review it and let me know if there are issues in it.
thank you.

juliagsy
juliagsy suggested changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

Hey Sarvesh! Thanks for the great work! I've left some comments below mainly related to support general additional support and refactor. The updated guidelines can also be found here: https://docs.google.com/document/d/1NfVngAQlQQlxLGEg47O2lnfn60M5xZekZFei9k5RJd8/edit?usp=sharing
Please feel free to lmk if you have any questions! Thanks!

ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models_tests/googlenet/test_googlenet.py Outdated Show resolved Hide resolved
juliagsy
juliagsy suggested changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@juliagsy juliagsy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey Sarvesh! Thanks for the great work! The code looks good to me overall and we're good to merge once the final comments are addressed and that the tests are passing! Thanks!
PS: please use "squash and merge" when merging :D

ivy_models/googlenet/layers.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
juliagsy
juliagsy suggested changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@juliagsy juliagsy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey Sarvesh! All dropout values should be 0 including aux_dropout. I've left some comments, once they are all in place, the test should pass for both load weights = True and False (I've tried it out :D) Thanks!
PS: Please also clean up the code after that and we'll then be able to look into merging, thanks!

ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models/googlenet/googlenet.py Outdated Show resolved Hide resolved
ivy_models_tests/googlenet/test_googlenet.py Show resolved Hide resolved
ivy_models_tests/googlenet/test_googlenet.py Outdated Show resolved Hide resolved
ivy_models_tests/googlenet/test_googlenet.py Outdated Show resolved Hide resolved
juliagsy
juliagsy reviewed Aug 28, 2023
View reviewed changes
Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

Hey Sarvesh! Thanks for the changes! The PR may not be ready yet, but just thought I'd leave a comment before I forget, please remove any changes unrelated to models and keep googlenet related updates only, thanks!

@Sarvesh-Kesharwani
Copy link
Contributor Author

Hey Sarvesh! Thanks for the great work! The code looks good to me overall and we're good to merge once the final comments are addressed and that the tests are passing! Thanks! PS: please use "squash and merge" when merging :D

Sure will take care from next time.

juliagsy
juliagsy suggested changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

Hey Sarvesh! I was about to merge it and doing a final check, but the pretrained weights loading result is not correct anymore. I'm not sure what changed (most likely the architecture and its config) but could you please double check (one thing is that I had to make fc with_bias=True for the weights mapping to map correctly but I didn't have to change anything previously to make the tests run)
I'm also pretty sure that during the last review, tests were passing with minimal change (the review comments), let me know once it's done, thanks!

@juliagsy
Copy link
Contributor

Hey! A quick update, seems like inception3 had a similar issue 🤔
some resnet tests are not passing anymore now which should be, could you try looking into this direction? maybe try to see if any module changed within ivy or if they have shared layers which may have some implementation change? thanks!

@Sarvesh-Kesharwani Sarvesh-Kesharwani requested review from juliagsy and removed request for a0m0rajab and juliagsy August 29, 2023 17:35
@Sarvesh-Kesharwani
Copy link
Contributor Author

Hey! A quick update, seems like inception3 had a similar issue 🤔 some resnet tests are not passing anymore now which should be, could you try looking into this direction? maybe try to see if any module changed within ivy or if they have shared layers which may have some implementation change? thanks!

Working on the resnet failure issue.

juliagsy
juliagsy approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

I'm temp commenting out logins value tests (the float dtype ones)
otherwise lgtm, thanks!

juliagsy
juliagsy approved these changes Sep 4, 2023
View reviewed changes
Copy link
Contributor

@juliagsy juliagsy 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!

@Sarvesh-Kesharwani Sarvesh-Kesharwani merged commit cdfee72 into ivy-llc:main Sep 4, 2023
10 of 15 checks passed
@Sarvesh-Kesharwani Sarvesh-Kesharwani deleted the gnetv1-2 branch September 4, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a0m0rajab a0m0rajab a0m0rajab left review comments

@juliagsy juliagsy juliagsy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sarvesh-Kesharwani @juliagsy @a0m0rajab