Deprecate PreProcessor

[djdameln]

Description

• This PR deprecates the PreProcessor class, which was already reduced to a wrapper around an albumentations transform object. With these changes we now just use albumentations transforms directly.
• Adds some configurability options for the default transforms:
  • Previously we always normalized to ImageNet stats if no transform_config was supplied, which was a bit problematic for models (e.g. DRAEM) that do not require this as a preprocessing step. The Normalization is now optional and can be disabled by setting config.dataset.normalize to false.
  • Center cropping can now be enabled from the config. This is useful to reproduce the results of models like PatchCore which have this in the original implementation. Center cropping can be enabled by setting config.dataset.center_crop to the desired crop size.

Changes

• Bug fix (non-breaking change which fixes an issue)
• Refactor (non-breaking change which refactors the code base)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist

• My code follows the pre-commit style and check guidelines of this project.
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing tests pass locally with my changes
• I have added a summary of my changes to the CHANGELOG (not for minor changes, docs and tests).

[jpcbertoldo]

ImageNet statistics

Using the torchfx extractor, the pre-trained model is using the new model versioning system in torchvision, no?

If that's the case, I think the preprocessing statistics necessary to each pre-trained model are encapsulated in the version object (?), so it shouldn't need to be assumed anywhere that it's from imagenet (which is often but not always the case).

[jpcbertoldo]

Another comment: I think consts/enums should be used instead of bools; if a new option for normalization shows up, it's natural to add a value in the enum (while the bool would need a refactor,).

Do you think there could be a use case where the normalize needs more than 2 values?
By contradiction, is it impossible that normalize needs more options than a bool?

[djdameln]

We default to ImageNet normalization because it's by far the most used. If a user needs custom normalization statistics they can configure it through the transform_config.

[djdameln]

Another comment: I think consts/enums should be used instead of bools; if a new option for normalization shows up, it's natural to add a value in the enum (while the bool would need a refactor,).

Good idea. I'll replace the bool with an enum :)

[ashwinvaidya17]

Thanks for the PR.

[jpcbertoldo]

picky comment: could you define InputNormalizationType = Union[InputNormalizationMethod, str] somewhere?

to be consistent with

FeatureExtractorParams = Union[TimmFeatureExtractorParams, TorchFXFeatureExtractorParams]

in #748

i'm actually wondering if this is necessary since InputNormalizationMethod inherits from str and Enum, but in any case it would be nice to avoid repetitive Union[...] for maintenance purposes

[djdameln]

i'm actually wondering if this is necessary since InputNormalizationMethod inherits from str and Enum

This is a good point, actually. This works fine:

>>> isinstance(InputNormalizationMethod.IMAGENET, InputNormalizationMethod)
True
>>> isinstance(InputNormalizationMethod.IMAGENET, str)
True

I guess the only question is if we want to allow users to instantiate the class with a string instead of the enum. So, do we want to allow something like this:

mvtec = MVTec(..., normalize="imagenet", ...)

If we do, then we need the Union typing.

[jpcbertoldo]

So, do we want to allow something like this:

mvtec = MVTec(..., normalize="imagenet", ...)

Unless there it makes handling things harder somehow, I'd say yes :)

[djdameln]

In that case we need to keep the str typing. For now we'll merge it as, so with the Union typing. We could consider switching to custom types later if needed.