Skip to content
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

Update timm to 1.* version and support more encoders #885

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

qubvel
Copy link
Collaborator

@qubvel qubvel commented Jun 8, 2024

Make most of the models from timm compatible with decoders (including transformers and convnext models)

@qubvel qubvel marked this pull request as draft June 8, 2024 23:32
Copy link

@wouterzwerink wouterzwerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I've left some suggestions that fixed the tests on my side, since I am looking to use segmentation_models_pytorch with timm>1.0

in_channels: int = 3,
depth: int = 5,
output_stride: int = 32,
out_indices: Optional[List[int]] = None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
out_indices: Optional[List[int]] = None,
out_indices: Union[str, List[int]] = "first",

Comment on lines +56 to +57
if "encoder_indices" in kwargs and kwargs["encoder_indices"] is None:
kwargs["encoder_indices"] = "first"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if "encoder_indices" in kwargs and kwargs["encoder_indices"] is None:
kwargs["encoder_indices"] = "first"
if "out_indices" in kwargs and kwargs["out_indices"] is None:
kwargs["out_indices"] = "first"

While it's called encoder_indices in the function select_feature_indices, in TimmUniversalEncoder it is still called out_indices

Comment on lines +69 to +73
encoder_indices = kwargs.pop("encoder_indices", None)
if encoder_indices is not None:
logger.warning(
"Argument `encoder_indices` is supported only for `tu-` encoders (Timm) and will be ignored."
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
encoder_indices = kwargs.pop("encoder_indices", None)
if encoder_indices is not None:
logger.warning(
"Argument `encoder_indices` is supported only for `tu-` encoders (Timm) and will be ignored."
)
out_indices = kwargs.pop("encoder_indices", None)
if out_indices is not None:
logger.warning(
"Argument `out_indices ` is supported only for `tu-` encoders (Timm) and will be ignored."
)

@qubvel
Copy link
Collaborator Author

qubvel commented Jun 24, 2024

Hi @wouterzwerink thanks for the review!
I am working on it, it requires a bit more in depth analysis to make most of the models compatible and configurable.

@adamjstewart
Copy link
Collaborator

According to #918, timm 1.0.9 already works great. Or our tests are useless. One or the other.

@qubvel
Copy link
Collaborator Author

qubvel commented Sep 12, 2024

This PR is more about unlocking newer backbones, the latest timm itself should work with the current setup

@patrontheo
Copy link

@qubvel @adamjstewart Is it planned to support the transformer backbones from timm anytime soon ? That would be awesome !!

@qubvel
Copy link
Collaborator Author

qubvel commented Dec 4, 2024

Hey @patrontheo, I've overcomplicated it a bit and can't find time to finish this 🥲 maybe during the Christmas holidays...
Do you have in mind specific architectures you would like to be supported?

@patrontheo
Copy link

Hey @patrontheo, I've overcomplicated it a bit and can't find time to finish this 🥲 maybe during the Christmas holidays... Do you have in mind specific architectures you would like to be supported?

I had in mind Swin but I think any good transformer backbone could help! Okay no worries, don't forget to take some time off during Christmas ;)

@qubvel
Copy link
Collaborator Author

qubvel commented Dec 4, 2024

Thanks @patrontheo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants