-
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
Load model did not work correctly as DFMModel did not inherit #5
Conversation
`nn.Module` - Change: `DFMModel` is now a subclass of `nn.Module` - Change: `SingleClassGaussian` is not a subclass of `DynamicBufferModule` - Fix: Missing/incorrect doc strings in `pca.py` and `dfm_model` - Fix: Load model test in `test_model.py` compares metrics for classification models. - Fix: Rename `SingleclassGaussian` to `SingleClassGaussian` - Fix: Incorrect input region in `generate_random_anomaly_image` in dummy dataset helpers
anomalib/models/dfm/dfm_model.py
Outdated
|
||
""" | ||
feats_orig = sem_feats.cpu().numpy() | ||
feats_orig = sem_feats |
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.
Similarly, this could be features
only. We wouldn't need to assign this to a new variable
|
||
|
||
class SingleclassGaussian: | ||
class SingleClassGaussian(DynamicBufferModule): |
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.
should we move this to anomalib/core/model/single_class_gaussian.py
?
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.
If it's used in multiple algos, then I'd agree. If this is the only algo, then it could stay here for now
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 think there are several other classes in core/model
that are also only used in one algo. KDE
is only used by DFKDE and MultivariateGaussian
is only used by PADIM. So for consistency I would be in favor of moving SingleClassGaussian
there as well. But if you disagree I'm also fine with keeping it here.
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.
When I was working on patchcore, I've moved all patchcore related models under patchcore directory.
If we want to keep them in one place, we could then move patchcore stuff there too.
In addition, I think we need to move them from anomalib.core.models
to anomalib.models.utils
or anomalib.utils.models
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.
In that case let's create a ticket for this.
Changes
DFMModel
is now a subclass ofnn.Module
SingleClassGaussian
is not a subclass ofDynamicBufferModule
pca.py
anddfm_model
test_model.py
compares metrics forclassification models.
SingleclassGaussian
toSingleClassGaussian
generate_random_anomaly_image
indummy dataset helpers