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 MobileNet v1 #140

Merged
merged 15 commits into from
Apr 4, 2022
Merged

Add MobileNet v1 #140

merged 15 commits into from
Apr 4, 2022

Conversation

darsnack
Copy link
Member

@darsnack darsnack commented Apr 2, 2022

Adds MobileNet v1 for completeness.

layers
end

function MobileNetv1(imsize::NTuple{2, Int} = (224, 224), width_mult = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function MobileNetv1(imsize::NTuple{2, Int} = (224, 224), width_mult = 1;
function MobileNetv1(imsize::Dims{2} = (224, 224), width_mult = 1;

Copy link
Member

Choose a reason for hiding this comment

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

Hmm currently all the models use NTuple{2, Int} so maybe we'd have to change that across all occurrences for uniformity

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure if there were any scenarios where Int could be something else (some static type, maybe), so I held off from commenting on the earlier model PRs as they left the eltype out. Assuming none of those changes are released, we could always constrain everything to dims and loosen it up if the need arises.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some which are not intended to be Dims but I left those as is. I did change the rest.

Copy link
Member Author

@darsnack darsnack Apr 2, 2022

Choose a reason for hiding this comment

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

There are one case (VGG) that has <:Integer and has been released, but hopefully this PR is straightforward enough to get in before v0.7 is released.

@darsnack
Copy link
Member Author

darsnack commented Apr 2, 2022

There's some mismatch between the "large" tests in #132 and here. Since the tests appear to pass the MobileNet section and error later, I'd like to merge this (assuming no changes requested) and deal with memory issues in #132.

@darsnack
Copy link
Member Author

darsnack commented Apr 4, 2022

Bump @ToucheSir?

@darsnack darsnack merged commit aba6fb8 into FluxML:master Apr 4, 2022
@darsnack darsnack deleted the mobilenet-v1 branch April 4, 2022 15:53
@darsnack darsnack mentioned this pull request Apr 4, 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