-
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
Anomalib CLI Improvements - Update metrics and create post_processing section in the config file #607
Anomalib CLI Improvements - Update metrics and create post_processing section in the config file #607
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.
Looks good to me. Maybe later we can look into setting up these values without using a callback.
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.
Maybe later we can look into setting up these values without using a callback.
Yeah. I'm a bit concerned about adding too many ConfigurationCallback
s, because I feel this is not in line with the intended use of PL's Callback
class. But at this point I also don't have a better design in mind that would work with the CLI. So for now we could keep it this way.
adaptive_threshold or default_image_threshold is not None and default_pixel_threshold is not None | ||
), "Default thresholds must be specified when adaptive threshold is disabled." | ||
|
||
self.adaptive_threshold = adaptive_threshold |
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 consider replacing this parameter with a selectable called thresholding_method
with the options fixed
and adaptive
. This might be a bit less confusing (for the unknowing reader, adaptive_threshold
could sound like it holds a threshold value). Later we could extend it with synthetic
option.
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 was also having a similar idea in my mind. I was not quite sure how exactly to address fixed image and pixel thresholds though
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 would be the equivalent of adaptive_threshold: false
in the current implementation
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.
yeah, but let's say we set thresholding_method
to fixed
. We would still need to set fixed_image_threshold
and fixed_pixel_threshold
, right?
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.
yeah, but I don't see any problems with that. If users want to use a fixed threshold then they will have to provide a value for that. If they don't know which value to provide then they should just use the adaptive threshold.
This is the phase-2 of this cli refactoring. It requires some additional extensions to the lightning cli |
Description
metrics
and createpost_processing
section in theconfig
file. #384Changes
Checklist