-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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 check to '_add_norm' #3820
Add check to '_add_norm' #3820
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
BTW, my first PR ever so apologies if done something silly 😳 |
Congrats on your first PR! :D You didn't do anything silly at all -- or at least, not anything that I can see. Many thanks for your contribution. |
Looks like there's a CI failure - if you have a moment, perhaps you could try running the notebook with the failing test to see what the issue is? |
Congrats on the first PR. Since we were discussing this issue on the fastai discord, I thought I would chime in after looking at the PR. I think the CI error is due to the new code preventing the automatic conversion of single channel greyscale images to three channel images when the user doesn't specify I think the solution is to add |
Right, so I have successfully broken things when trying to fix things... 🤣 Will go with @warner-benjamin suggestion and add What should be the behaviour in such a case though? Not applying normalization to the inputs of ImageNet pertained model goes against 'common practice' (for lack of better phrase) but not sure if it actually is a problem - the pixels are between 0 and 1 (so they don't take on crazy values like -1234), the networks have normalisation layers everywhere... On the other hand inserting a 1-channel normalisation transform with stats based on say the 1st batch might not be the worst idea. I mean, why are we normalizing with ImageNet stats to begin with? If a pertained model expects 0-mean 1-stdev inputs, then using ImageNet stats to normalize say x-ray images quite likely doesn't give you what you want. Although I suppose you at least don't have to calculate the stats for your dataset yourself. So many questions! 🤯 |
Since it sounds like we're not really sure what the desired behavior is, perhaps I should close this PR, and we can discuss it on the forums or discord, if that's OK? Generally it's best to use the same normalisation constants used when training the pretrained model, which is why we use that as the default. |
Actually on further consideration I think this PR as is can only be an improvement on the current situation. I wouldn't say I fully understand all the potential impacts though. What do you both think? |
It definitely is an improvement. I have created a new thread on the forum https://forums.fast.ai/t/add-check-to-add-norm-3820-pr/101344 as this whole normalisation business is definitely worth discussing. I will also run some experiments comparing norm using imagenet_stats vs actual stats from the dataset being used (particularly when they're different from imagined_stats like MNIST) vs no normalisation at all (with freezing/unfreezing of batch norm layers, which should be able to fix any issues with non normalised inputs...maybe). |
OK cool I'll merge this now then, but feel free to do followup PRs if you have more ideas. |
In the code above, the
unet_learner
will automatically add a normalization transform usingimagenet_stats
. As a result, the dataloader will start producing 3-channel images, although the first conv layer of the UNet now has 1 input channel.This results in an exception being thrown when one tries to train. The same happens with
vision_learner
.Wasn't sure what the best way to address this is; in this PR I just throw a warning and don't add the normalization. Another possibility would be to absorb the normallization statistics into the weights and biases of the first conv layer, but this
doesn't go well with zero-padding (would work as if you padded with zeros yourself and then normalized these zeros too).