Refactor feature extraction key

[ashwinvaidya17]

Description

• All backbone/feature extraction keys are now in

model:
    feture_extractor:
        backbone:
        layers:

• FeatureExtractor selects timm or torchfx based on the parameters passed to it.
• This PR ended up way bigger than I thought 😃

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

[jpcbertoldo]

great effort, thanks!

[djdameln]

Thanks. I had a close look at the design and I have some concerns, which are outlined below, along with some minor comments.

[samet-akcay]

Similar to what @djdameln raised, my primary concern is the following line in torch_model.py and lightning_model.py:

feature_extractor: FeatureExtractorParams

feaature_extractor variable actually contains feature extractor parameters. Storing these as feature_extractor causes a confusion since the codebase has so many feature_extractor here and there.

[samet-akcay]

This is way too much effort for a part that we plan to deprecate. Really appreciate your effort @ashwinvaidya17 !

[djdameln]

Thanks, huge improvement now that one layer of wrapper has been removed. I am not 100% convinced that we need the wrapper design at all, since the functionality added by the wrapper classes is only used for the construction of the model. But to speed things up, let's stick with this design for now. We can revisit the design once we deprecate timm.

[jpcbertoldo]

Is there a github discussion/issue from which one can read why

"TimmFeatureExtractor will be removed in the future version."

?

[samet-akcay]

Is there a github discussion/issue from which one can read why

"TimmFeatureExtractor will be removed in the future version."

?

@jpcbertoldo, we have recently spotted that timm feature extractor provides inconsistent results, which overall adversely impact the performance of the models. We therefore decided to use torchfx feature extractor

[ashwinvaidya17]

Is there a github discussion/issue from which one can read why

"TimmFeatureExtractor will be removed in the future version."

?

I've gathered our motivation here #779

[djdameln]

Thanks. Just two minor comments left:

[djdameln]

Should this module be called feature_extraction? This would be more in line other modules under models.components (e.g. dimensionality_reduction and sampling)

[samet-akcay]

Thanks, only a single comment. Thanks a lot for your effort!

[djdameln]

Thanks!