-
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
Create Anomalib CLI #378
Create Anomalib CLI #378
Conversation
…o nb/sa/add-dataset-module-notebook
…notoolkit/anomalib into nb/sa/add-dataset-module-notebook
…o nb/sa/add-dataset-module-notebook
…o samet-akcay/issue21
Can you add commands to run the models to the README? |
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 efforts. From the little exploration I've done, this is a cleaner approach.
|
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!
model_name = config.model.class_path.split(".")[-1].lower() | ||
data_name = config.data.class_path.split(".")[-1].lower() | ||
category = config.data.init_args.category if config.data.init_args.keys() else "" | ||
time_stamp = datetime.now().strftime("%Y-%m-%d_%H-%M-%S") |
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.
What do you think about this time_stamp
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.
I don't see an issue with it.
parser.set_defaults( | ||
{ | ||
"metrics.adaptive_threshold": True, | ||
"metrics.default_image_threshold": None, | ||
"metrics.default_pixel_threshold": None, | ||
"metrics.image_metric_names": ["F1Score", "AUROC"], | ||
"metrics.pixel_metric_names": ["F1Score", "AUROC"], | ||
"metrics.normalization_method": "min_max", | ||
} | ||
) |
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'm probably misunderstanding something, but why do we need to set the defaults? Aren't those parsed from the config?
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 they are provided in the config, yes, they are parsed from the config file. However, if the user does not provide those parameters from the config file, then this part sets these by default. With this approach it becomes also possible to configure these from the CLI as well.
anomalib/utils/cli/cli.py
Outdated
} | ||
) | ||
|
||
def before_instantiate_classes(self) -> None: |
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 quite long and therefore a bit hard to navigate. Do you think you could split it between several smaller methods?
# TODO: This is a workaround. normalization-method is actually not used in metrics. | ||
# It's only accessed from `before_instantiate` method in `AnomalibCLI` to configure | ||
# its callback. | ||
self.normalization_method = normalization_method |
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.
Not sure I understand this. Maybe we could discuss it in the daily
Description
This is to add new anomalib cli that is based on pytorch lightning cli. With this design it'll be possible to tweak the parameters from both config files and cli simultaneously.
Fixes Refactor Anomalib CLI #21
Changes
Checklist