Skip to content

Conversation

johnsutor
Copy link
Contributor

This is a WIP implementation of the SparK algorithm, as requested by #1462

@guarin
Copy link
Contributor

guarin commented Dec 18, 2024

Thanks a lot for the PR! The review might take a while due to the upcoming holiday season.

Copy link
Contributor

@guarin guarin left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! This will take a while to get merged as it contains many changes. I left a lot of comments/questions regarding the various parts. Let us know if you need help, we can also support you in making the changes or take over parts of the PR if that helps.

SparseMaxPooling,
SparseSyncBatchNorm2d,
SparseConvNeXtBlock,
SparseConvNeXtLayerNorm
Copy link
Contributor

Choose a reason for hiding this comment

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

SparseConvNeXtLayerNorm is imported but not used

)


class SparseConvNeXtBlock(nn.Module):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between this ConvNeXt implementation and the ones from torchvision and timm? Would it be possible to inherit from torchvisions CNBlock instead as it is used in dense_model_to_sparse? This way we wouldn't have to implement DropPath ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was initially opting to not rely on Timm, but I'm fine with working with Timm for simplicity. Will change!

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move code in this file to lightly/models/modules/spark_sparse.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the code in this file to lighlty/models/modules/spark_sparse.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add this emtpy file

@johnsutor
Copy link
Contributor Author

Thanks for all the comments! I'll make some changes later today and I'll let you know how much help I'll need given the scope of this project.

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.

2 participants