Skip to content
This repository was archived by the owner on Apr 23, 2025. It is now read-only.

Conversation

@brettkoonce
Copy link
Contributor

No description provided.

@rxwei rxwei requested a review from jekbradbury May 9, 2019 04:02
Copy link
Contributor

@saeta saeta left a comment

Choose a reason for hiding this comment

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

This looks awesome! I'll let @jekbradbury review as well, but thank you very much.

Couple quick thoughts:

  1. How does the compile time of swift-models change with the new version of ResNet? :-D
  2. To simplify things a bit, could you make imageSize optional (either in this PR or a follow-on PR)? ResNet is fully convolutional and works with multiple image sizes without changing the model. Having "imageSize" be a required parameter (and a single scalar instead of a (height, width) tuple) might imply to people that the ResNet implementation only works for a single (square) image size. (Thus discouraging folks from using techniques like progressive resizing.)
  3. In a follow-up PR, let's discuss making the ResNet implementation public so that folks can (in their own Swift projects or notebooks) import the canonical ResNet and use it themselves.

Thank you again. This is super awesome to see.

All the best,
-Brennan

@brettkoonce
Copy link
Contributor Author

re 1) Not sure if this is just my system (running semi-current tf-swift master), but the existing version in models repo takes 43 seconds to compile here. Using this code takes ~3-4 seconds to compile.
re 2) Not sure how the current codebase handles arbitrary input, but certainly happy to rework things however desired.
re 3) That is fine, but having said that it would probably be good to refactor this a bit more first. If there is a way to parameterize the BasicBlock/ResidualIdentityBlock logic then this file could be cut in half again and everything reduced to a single ResNet class, which I think would be simpler for people new to computer vision. This torchvision logic is nice: https://github.com/pytorch/vision/blob/50d54a82d1479ffb6dd7469ed05fccdf290a1d84/torchvision/models/resnet.py#L216

Let me know!

@rxwei
Copy link
Contributor

rxwei commented May 10, 2019

re 1) Not sure if this is just my system (running semi-current tf-swift master), but the existing version in models repo takes 43 seconds to compile here. Using this code takes ~3-4 seconds to compile.

This is expected because the compiler is bugged. The implementation detail is that we've turned off a part of the Swift compiler that optimizes large structures in order to suppress other bugs. We are actively working on this and expect this to be fixed within a week or two.

@jekbradbury
Copy link
Contributor

2. could you make imageSize optional

This would be a good change. The reason imageSize exists at all is to distinguish between ResNets designed for CIFAR image sizes and those designed for ImageNet sizes; my impression is that the ImageNet-sized networks also work fine for images of other similar sizes.

@brettkoonce
Copy link
Contributor Author

I can work on coming up with a way to add an input type to specify cifar, imagenet and generic variants. In the same vein, can come back to refactoring things more down the road.

@brettkoonce brettkoonce force-pushed the resnet-block-cleanup branch from 56371e4 to 6ed2af6 Compare May 14, 2019 00:09
case resNet152
}

init(kind: Kind, type: InputKind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If kind conflicts with the first argument label, you can call it inputKind:. Relatedly, would DataKind be a better name for the type? If so, dataKind: would be a good label name. Same for other initializers that take DataKind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to: inputKind: Kind, dataKind: DataKind, let me know what you think!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants