-
Notifications
You must be signed in to change notification settings - Fork 19.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 ConvNeXt models #16421
Add ConvNeXt models #16421
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 for the PR! This is a great addition to applications
.
Comments may not be exhaustive.
Once the implementation looks good, I will add the other components. Please find the model conversion and the evaluation details in this comment: #16421 (comment). By "conversion" I mean the following:
Implementation correctness needs to be ensured in these cases, hence the evaluation. |
keras/applications/convnext.py
Outdated
"xlarge": | ||
("da65d1294d386c71aebd81bc2520b8d42f7f60eee4414806c60730cd63eb15cb", | ||
"2bfbf5f0c2b3f004f1c32e9a76661e11a9ac49014ed2a68a49ecd0cd6c88d377"), | ||
} |
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.
I have also converted the ImageNet-21 checkpoints which would likely be better for transfer learning than the checkpoints from ImageNet-1k pre-training.
But to add those checkpoints we need the following:
- ImageNet-21k models are supposed to be multi-label classifiers. So the activation should be "sigmoid". So when
weights="imagenet21k" && include_top=True
,classifier_activation
is supposed to besigmoid
.
This would require changes to imagenet_utils.validate_activation
. As I understand it, it only supports softmax
at the moment.
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.
IMO, a better idea would be to keep the PR as it is. Once it's done, we could work on another PR setting up validation for sigmoid
when loading pre-trained models like the one mentioned above. After that, I'm happy to work on another PR to incorporate the ImageNet-21k checkpoints making the changes necessary.
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.
Sounds good, it's fine to only have support for imagenet1k for now (this is consistent with the other applications). We can add more checkpoints in the future.
keras/applications/convnext.py
Outdated
"xlarge": | ||
("da65d1294d386c71aebd81bc2520b8d42f7f60eee4414806c60730cd63eb15cb", | ||
"2bfbf5f0c2b3f004f1c32e9a76661e11a9ac49014ed2a68a49ecd0cd6c88d377"), | ||
} |
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.
Sounds good, it's fine to only have support for imagenet1k for now (this is consistent with the other applications). We can add more checkpoints in the future.
@fchollet just incorporated the changes you asked for. These changes will require a re-conversion of the pre-trained parameters since the model structure has been changed a bit now. I will do that (as well as the ImageNet-1k evaluation) after others have had a chance to review the PR. |
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.
Looks good to me - just need to figure out the right end action per Francois' comments and add tests when that is done. Thanks @sayakpaul for the contribution!
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.
Performed a more thorough pass to get this moving along. A few minor changes, then a big question regarding normalization that I think @fchollet will have context on.
|
||
def apply(x): | ||
x = layers.Normalization( | ||
mean=[0.485 * 255, 0.456 * 255, 0.406 * 255], |
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.
So, these are initialized based on imagenet: this is required for use with the pretrained weights. Is there a way we can allow users to configure this for custom datasets?
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's inspired from ResNet-RS and RegNets. For non-ImageNet weights, it's necessary to disable it.
@LukeWood with the current setup, we have a problem. https://github.com/sayakpaul/keras/blob/feat/convnext/keras/applications/applications_test.py#L129 will fail being unable to get instantiated from the config. https://github.com/sayakpaul/keras/blob/feat/convnext/keras/applications/applications_test.py#L134-#L137 does not help. Error trace (used separately to test the component separately):
I am currently trying to wrap LayerScale as a separate layer so that If this works out well, then the problem is solved otherwise we'll have to brainstorm more. |
feat: convnext with functional api.
@LukeWood I think I was able to make things work. I ran Here's what changed:
One thing I couldn't understand is that without this Let me know your thoughts on the recent changes. P.S.: Weight conversion and verification have been performed on ImageNet-1k as well and they are all good. Refer here. |
chore: spacing fix/
Hey @sayakpaul ! I uploaded the weights to our bucket. Now we can update the path in your code and merge the PR. Thanks! |
@LukeWood done. Thank you! |
This is great to have in |
super excited to have it in keras.applications! |
Closes #16321
Conversion scripts and ImageNet-1k evaluation are available here: https://github.com/sayakpaul/keras-convnext-conversion.
Comparison to the actual reported numbers
@LukeWood