-
Notifications
You must be signed in to change notification settings - Fork 5
Adding Matmul condition on srs size and cqlin setup assertions (Issue #136) #171
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
base: master
Are you sure you want to change the base?
Adding Matmul condition on srs size and cqlin setup assertions (Issue #136) #171
Conversation
Vimalakirti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contributions! I left some comments.
added new empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing to our codebase! CQLinBB is actually used in multiple types of layers (e.g., Conv, Matmul, Conv2d). I think instead of directly embedding the logic into the Matmul layer I think it would be better to make a helper function with this logic and call it for each usage of CQLinBB. This would also make testing cleaner, as we would be able to test the function directly instead of copy-pasting the code in the test.
| let b = 64; | ||
| let matrix_size = a * b; | ||
|
|
||
| let input_vec_fr: Vec<Fr> = input_values.iter().map(|&x| f32_to_fr(x, 10)).collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify the test by generating a random Fr input and weights instead of loading floats from a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to contribute to the project. I will change this logic into a helper function to apply to the other layers as soon as I get some free time.
Regarding simplifying the test, I thought that loading from a file would be more realistic feeding directly the files but shouldn't be a concern if changed that way.
Will do.
Added the constraints for issue #136 and tests