-
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
Refactor AnomalyModule
and LightningModules
to explicitly define class arguments.
#315
Conversation
…ization callbacks
…o refactor/sa/lightning-modules
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! I have a few minor comments.
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. One concern that I'm having with these configuration callbacks, is that it might confuse the user. MetricsCallback
may sound like it is responsible for collecting the outputs and computing the performance metrics, while in practive it is only used to initialize the metric collections. When more callbacks like this get added, it might be good to make a clear distinction between these configuration callbacks and the other callbacks. Maybe we could keep them in a separate folder and/or add a postfix to the class names, e.g. MetricsConfigurationCallback
.
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 am fine with the changes.
@djdameln, if you are happy with the final changes we could merge this PR today |
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.
Great work, thanks!
Description
AnomalyModule
andLightningModules
to explicitly define class arguments. Current design depends onhparams
variable, which is aDictConfig
. Despite being convenient, it is not explicit such that the user may not now what exactly needed to construct a model class. The proposed implementation addresses this explicitly construct the model classes.Changes
Checklist