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

Fix partition hashing #338

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Conversation

Al-Kindi-0
Copy link
Contributor

This is a small fix for the condition under which partitioned hashing when committing is enabled.

Comment on lines 378 to 380
/// Returns the size of each partition used when committing to the main and auxiliary traces as
/// well as the constraint evaluation trace.
pub fn partition_size<E: FieldElement>(&self, num_columns: usize) -> usize {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to clarify in the comment that the returned number of columns is the number in the field E (rather than the base field of E).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, added

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left one small comment inline. Also, could we rebase this on the main branch?

@Al-Kindi-0 Al-Kindi-0 changed the base branch from next to main October 30, 2024 08:28
@Al-Kindi-0
Copy link
Contributor Author

I have now changed the base to main

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you!

@irakliyk irakliyk merged commit be61c26 into facebook:main Oct 30, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants