Skip to content

Conversation

@NiDimi
Copy link
Collaborator

@NiDimi NiDimi commented Feb 10, 2025

-> As part of the benchmark tests I had to remove the compatibility with Sonobe (e.g. the R1CS carry overs) to avoid adding too much useless code

Comment on lines 175 to 176
// Right now we are packing the 4 matrices in a single vector to achieve compatibility with NIFStrait.
fn new_witness(abce: Vec<C::ScalarField>, e_len: usize, _rng: impl RngCore) -> Self::Witness {
fn new_witness(abce: Vec<C::ScalarField>, e_len: usize, _rng: impl RngCore) -> Witness<C> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we don't need to be compatible with the trait anymore, this should be updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

Besides this, I think the PR it's ready. But we should either tag the older implementation that implemented the trait, or keep this modified version on a different branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the _rng and I created the branch compatible_sonobe_version. I think it makes more sense as a branch since we might want to continue working on it in the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

@NiDimi Besides the _rng variable, I also meant the fact that we are passing abce as a single vector. I think each one should be a different variable, since we are no longer constrained on the number of parameters.

@NiDimi
Copy link
Collaborator Author

NiDimi commented Feb 24, 2025

#8 Adds all the changes here along with the inclusion of Matrex. Let's just keep this branch however so we can see differences in times between the original implementation and the refactored one.

@NiDimi NiDimi closed this Feb 24, 2025
@NiDimi NiDimi deleted the bench/mova_matrix branch February 24, 2025 21:18
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.

3 participants