[Custom loops POC] Support model loading #996
Conversation
Note: Breaks the sweep and benchmarking. This is temporary
| @@ -26,7 +26,7 @@ def _evaluation_step_end(self, *args, **kwargs) -> STEP_OUTPUT | None: | |||
| if outputs is not None: | |||
| self.trainer.post_processor.apply_predictions(outputs) | |||
| self.trainer.thresholder.update(outputs) | |||
| self.trainer.normalizer.update_metrics(outputs) | |||
| self.trainer.normalizer.update(outputs) | |||
There was a problem hiding this comment.
not sure if this method name is descriptive enough. I'll need to go to the definition to check what update really does to outputs
There was a problem hiding this comment.
How about update_normalization_statistics?
| self.trainer.thresholder.initialize() | ||
| self.trainer.metrics_manager.initialize() |
There was a problem hiding this comment.
It may not be relevant to the scope of this PR, but I see that Trainer object has normalizer which is a NormalizationManager. To follow the same concept, would it be an idea to have self.trainer.metrics, which would be a MetricsManager?
I feel this would be in line with other attributes such as trainer.post_processor, trainer.normalizer, trainer.metrics, rather than using Manager? Or vice-versa.
Thoughts? @djdameln, @ashwinvaidya17 ?
There was a problem hiding this comment.
Yeah, consistency would be better. I am fine with metrics.
There was a problem hiding this comment.
I would also prefer metrics.
| if not hasattr(self.trainer.lightning_module, "normalization_metrics"): | ||
| self.trainer.lightning_module.normalization_metrics = self.metric_class() | ||
| # Every time metric is called, it is moved to the cpu | ||
| return self.trainer.lightning_module.normalization_metrics.cpu() |
There was a problem hiding this comment.
Would there be an edge case where the user wants this to be on GPU?
There was a problem hiding this comment.
I think this is somethig might have to address later. We move the metrics and outputs to cpu at a bunch of places. We have this PostProcessor._outputs_to_cpu(outputs) in post-processor. I guess the original idea was to avoid oom issues by moving metrics computation to the cpu. But in this case we can look into adding a new key to the config to allow gpu computation.
src/anomalib/trainer/trainer.py
Outdated
| @@ -62,5 +62,5 @@ def __init__( | |||
| manual_pixel_threshold=manual_pixel_threshold, | |||
| ) | |||
| self.post_processor = PostProcessor(trainer=self) | |||
| self.normalizer = Normalizer(trainer=self, normalization_method=normalization_method) | |||
| self.normalizer = NormalizationManager(trainer=self, normalization_method=normalization_method) | |||
There was a problem hiding this comment.
The terminology is a bit confusing here. I would suggest calling this attribute self.normalization_manager to be more in line with the metrics_manager and avoid confusion with the Normalizer classes. This would also avoid trainer.normalizer.normalizer.
| Returns: | ||
| BaseNormalizer: Normalizer class | ||
| """ | ||
| normalization_method: BaseNormalizer | None = None |
There was a problem hiding this comment.
Maybe normalizer instead of normalization_method? Since the object is an instance of BaseNormalizer, not NormalizationMethod.
| if "anomaly_maps" in outputs.keys(): | ||
| outputs["anomaly_maps"] = cdf.standardize( | ||
| outputs["anomaly_maps"], stats.pixel_mean, stats.pixel_std, center_at=stats.image_mean | ||
| ) |
There was a problem hiding this comment.
This is another piece of logic that could be handled based on task_type. (Not for this PR, just wanted to point it out)
|
I've updated the PR with partial changes while we finalize the naming convention regarding the normalizer and metrics manager. |
| self._load_metrics() | ||
|
|
||
| def _load_metrics(self) -> None: | ||
| """Loads the thersholding and normalization classes from the checkpoint.""" |
There was a problem hiding this comment.
Not sure if it's in the scope of this PR, but export functionality is broken, because it tries to retrieve threshold information from the lightning model, which is now stored in trainer.
Description