-
Notifications
You must be signed in to change notification settings - Fork 709
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 FastFlow Model #336
➕ Add FastFlow Model #336
Conversation
…o feature/sa/fastflow
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! I have a few comments, mainly minor
anomalib/models/fastflow/config.yaml
Outdated
image_size: 256 # options: [256, 256, 448, 384] - for each supported backbone | ||
train_batch_size: 32 | ||
test_batch_size: 32 | ||
num_workers: 0 |
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.
num_workers
is usually set to a higher default value, for the other models it's 36.
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 spotting this
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 like its not consistent across algos.patchcore is 0 as well. I'll fix them all
|
||
self.feature_extractor.eval() | ||
if isinstance(self.feature_extractor, VisionTransformer): | ||
x = self.feature_extractor.patch_embed(x) |
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 pylint allow single letter variable names?
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 temporarily ignore that in L192. As I said, this FastFlowModel
would require some refactor later.
) | ||
self.anomaly_map_generator = AnomalyMapGenerator(input_size=input_size) | ||
|
||
def forward(self, x: Tensor) -> Union[Tuple[List[Tensor], List[Tensor]], Tensor]: |
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.
This method is a bit hard to read. Maybe you could split it into several sub-methods. Something like:
if isinstance(self.feature_extractor, VisionTransformer):
features = self.get_transformer_features(x)
elif isinstance(self.feature_extractor, Cait):
features = self.get_cait_features(x)
else:
features = self.feature_extractor(x)
hidden_variables = self.get_hidden_variables(features)
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! Only a few minor comments
("dfkde", False), | ||
("dfm", False), | ||
("stfpm", False), | ||
("fastflow", False), |
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 think this will work as the performance thresholds for fastflow haven't been updated in the performance threshold file.
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.
One minor issue but that can be addressed later. LGTM
| Transistor | 0.938 | 0.979 | 0.993 | 0.983 | | ||
| Wood | 0.978 | 0.992 | 0.979 | 0.989 | | ||
| Zipper | 0.878 | 0.951 | 0.981 | 0.977 | | ||
| Average | | | | | |
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.
Can you add the average score? Also, a minor issue but this format is a bit different from our other models (rows <-> columns). Maybe for consistency we should decide on a single format. Maybe this can be addressed in a different 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.
There are some categories that I need to run again to exactly produce the numbers, which is the reason why I left it blank. As agreed, I could add these later.
Regarding the table format, I would prefer this because 15 MVTec categories doesn't fit to the screen when they are placed into columns.
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.
We could perhaps modify all the tables once we have all the benchmark results merged
Description
Add fastflow implementation
Fixes Add FastFlow Model #261
Changes
Checklist