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

Reimplement cc_attention using einops. #1186

Closed
wants to merge 6 commits into from
Closed

Reimplement cc_attention using einops. #1186

wants to merge 6 commits into from

Conversation

Leojc
Copy link
Contributor

@Leojc Leojc commented Jul 9, 2021

Motivation

Reimplement a faster and more readable cc_attention using einops.

Modification

  1. Rewrite cc_attention.py
  2. Add einops to the requirement.txt

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2021

CLA assistant check
All committers have signed the CLA.

@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Jul 9, 2021

Hi @Leojc

  1. Do you have some statistic numbers that verifies the current implementation?
  2. I understand that the CUDA ops might be replaced, but the interfaces of this module should not be changed, e.g., in_channels should not be changed to in_dims.

@Leojc
Copy link
Contributor Author

Leojc commented Jul 9, 2021

Hi @ZwwWayne ,
1.
(1) I calculate the time for one forward (input size is [2,512,97,97]). The CUDA version is 0.0299619 and my implementation is 0.00544943. So about 5x faster.
(2) I also test it in https://github.com/open-mmlab/mmsegmentation. I replace the cc_attention of ccnet in mmsegmentation. Then I run with a config of ccnet_r50-d8_769x769_40k_cityscapes.py. The CUDA version gets 78.46 mIoU, and this implementation gets 78.99.
2. Sorry about that! I just fixed it.

@ZwwWayne ZwwWayne requested a review from xvjiarui July 10, 2021 00:12
@ZwwWayne
Copy link
Collaborator

Thank you @Leojc , the statistical results are important. You may also highlight the speed comparison in the docstring of the module. I will invite @xvjiarui to have look.

@hellock
Copy link
Member

hellock commented Jul 15, 2021

Can it be implemented with torch.einsum so that we can omit a third-party dependency?

@Leojc
Copy link
Contributor Author

Leojc commented Jul 15, 2021

Can it be implemented with torch.einsum so that we can omit a third-party dependency?

Yes, but that will decrease the readability greatly. Is that OK?

@Leojc
Copy link
Contributor Author

Leojc commented Jul 15, 2021

Can it be implemented with torch.einsum so that we can omit a third-party dependency?

Yes, but that will decrease the readability greatly. Is that OK?

Wait... Maybe that won't decrease too much. I'll try implementing it.

@Leojc
Copy link
Contributor Author

Leojc commented Jul 15, 2021

Think I should open a new PR.
Reimplement cc_attention using pure PyTorch #1201

@hellock
Copy link
Member

hellock commented Jul 15, 2021

Think I should open a new PR.
Reimplement cc_attention using pure PyTorch #1201

Looks great!

@Leojc Leojc closed this Aug 28, 2021
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