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

Swap axes order -- data flow (C)FKB #103

Merged
merged 2 commits into from
Jul 9, 2023
Merged

Swap axes order -- data flow (C)FKB #103

merged 2 commits into from
Jul 9, 2023

Conversation

lkct
Copy link
Member

@lkct lkct commented Jul 9, 2023

Based on #91, we should switch to a more efficient axes order.

The inner data flow is now (C)FKB, while the input and output of the model are BK which may be more intuitive. (output is contiguous as KB and a BK view is returned)

The input layer is not yet modified due to #93.

@lkct lkct added the enhancement New feature or request label Jul 9, 2023
@lkct lkct self-assigned this Jul 9, 2023
@lkct
Copy link
Member Author

lkct commented Jul 9, 2023

Benchmark results: 0.106/5.0

On the other hand, (C)FBK is 0.118/5.0, and therefore not chosen. (PS: with CFBK input, the einsum in mixing layer outputs non-contiguous tensor with all params configuration)

@lkct lkct merged commit 63e9140 into main Jul 9, 2023
@lkct lkct deleted the axes_order branch July 9, 2023 02:58
@arranger1044
Copy link
Member

This is a nice 2x speed up improvement!
As a ref, in Antonio's experiments he was using collapsed CP, this might explain why he was (slightly) faster.

@loreloc
Copy link
Member

loreloc commented Jul 9, 2023

Do we have benchmark results with a higher batch size? I feel that the choice of ...KB and ...BK depends on the batch size. I think ...BK is a more natural layout for our tensors. Moreover, this would remove the need of the final transpose on the circuit's output.

@loreloc loreloc mentioned this pull request Jul 9, 2023
@lkct
Copy link
Member Author

lkct commented Jul 9, 2023

yes we should add more configurations to benchmark, and that's why I'm keeping a bak branch for FBK

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

Successfully merging this pull request may close these issues.

3 participants