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.

Sign up for GitHub

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

Jump to bottom

Feature/sg 849 add replace in channels #1557

Merged
merged 37 commits into from
Oct 29, 2023
Merged

Feature/sg 849 add replace in channels #1557

merged 37 commits into from
Oct 29, 2023

Conversation

Louis-Dupont
Copy link
Contributor

@Louis-Dupont Louis-Dupont commented Oct 21, 2023 •
edited
Loading

Adding replace_input_channels and get_input_channels to SgModel (and other required stems/backbones)

@Louis-Dupont Louis-Dupont marked this pull request as ready for review October 21, 2023 20:52
@shaydeci
Copy link
Contributor

shaydeci commented Oct 23, 2023 •
edited
Loading

Adding replace_input_channels and get_input_channels to SgModel (and other required stems/backbones)

Question

How to handle models that were implemented with self.in_channels attribute, since we now have get_input_channels() method that dynamically checks the value without relying on an attribute?

a. Should we remove any self.in_channels attribute?

pro: cleaner IMO con: risk of not being backward compatible if models are built on top of these models.

b. Or should we update the value of self.in_channels when calling replace_input_channels() ? pro: 100% backward compatible con: risk of bug if we miss one place where self.in_channels should be updated con: redundant and maybe a bit more confusing

Most important thing this PR is missin, is a mechanism to replace the stem out of the box when using pretrained weights.
One should be able to use pretrained checkpoints, replacing the stem behind the scenes.

@Louis-Dupont
Copy link
Contributor Author

Louis-Dupont commented Oct 24, 2023 •
edited
Loading

Most important thing this PR is missin, is a mechanism to replace the stem out of the box when using pretrained weights.
One should be able to use pretrained checkpoints, replacing the stem behind the scenes.

Added here 287b28f

@Louis-Dupont
Copy link
Contributor Author

Added update of self.in_channels when calling replace_input_channels: ab760e8

@Louis-Dupont Louis-Dupont force-pushed the feature/SG-849-add_replace_in_channels branch from 44d9e93 to df11873 Compare October 29, 2023 13:24
shaydeci
shaydeci approved these changes Oct 29, 2023
View reviewed changes
Copy link
Contributor

@shaydeci shaydeci left a comment

Choose a reason for hiding this comment

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

LGTM

@shaydeci shaydeci merged commit c0594a3 into master Oct 29, 2023
3 checks passed
@shaydeci shaydeci deleted the feature/SG-849-add_replace_in_channels branch October 29, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BloodAxe BloodAxe BloodAxe left review comments

@shaydeci shaydeci shaydeci approved these changes

@ofrimasad ofrimasad Awaiting requested review from ofrimasad ofrimasad is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Louis-Dupont @shaydeci @BloodAxe