-
Notifications
You must be signed in to change notification settings - Fork 705
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 Reverse Distillation #343
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, good to see this model being added to the repo. The torch model seems to contain some classes that were directly copied from torchvision with some minor changes. Do you think it would be possible to import the models from torchvision instead, and replace the specific parts? It would keep our code base a bit cleaner.
inplanes (int): Number of input channels. | ||
planes (int): Number of output channels. |
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.
do you know why these are called in/out planes. Is it how they are named in the paper?
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 don't know why. But the code in RD is taken from resent in torchvision so based on conv parameters, I've added this docstring https://github.com/pytorch/vision/blob/main/torchvision/models/resnet.py#L64
cosine_similarity is not supported op in onnx
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.
only minor stuff...
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. Some minor comments
@@ -68,6 +68,8 @@ def forward(self, images: Tensor) -> Union[Tensor, Tuple[List[Tensor], List[Tens | |||
Union[Tensor, Tuple[List[Tensor],List[Tensor]]]: Encoder and decoder features in training mode, | |||
else anomaly maps. | |||
""" | |||
self.encoder.eval() |
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.
Are the benchmarking results still the same after this change?
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 am running it right now. But that was a good catch. I saw that the encoder was in training mode in the train step.
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.
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.
The average results also seem to be a bit lower than those reported in the original paper (98.5% image AUROC, 97.8% pixel AUROC). For now I would suggest to merge this PR, but it would be good to investigate if this difference can be explained.
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.
Does the official implementation give the exact same results reported in the paper?
Description
Add Reverse Distillation model based on official code - https://github.com/hq-deng/RD4AD
Fixes Add Reverse Distillation Model #263
TODO