Documentation and minor robustness changes #14

GottfriedHerold · 2024-11-04T15:18:39Z

This PR mostly adds documentation changes that I wrote while trying to understand the codebase.
It intentionally does not do functional changes.

The only actual code changes are additions of some assert! guards, adding some extra tests
and splitting some functions more cleanly into parallel / non-parallel versions.
The latter is done to simplify testing and enable differential tests (which the old version could not do, because only one version was compiled).

- Document behaviour of to_binary (specify the endianness and what happens if value > 2**n_bits). - Add test for to_binary - Document base_decomposition

…more precise what they do, because there is some ambiguity from the name alone.

…al and avoids shadowing a variable name.

…r accordingly), because it lacks a bounds-check for the given indices.

…Mut::ptr_at_mut. These bounds-check are repeated in the respective sole caller (which is not unsafe). Alternatively, we could remove declare ptr_at and ptr_at_mut as *not* unsafe.

ntt/utils.rs: added tests and improved documentation ntt/utils.rs: Changed lcm(a,b) slightly to avoid a potential overflow of an intermediate result. Now it overflows iff the result overflows.

…n split into two versions (parallel | non-parallel) rather than an inline #[cfg], because that makes it hard to write unit tests.

…worked tests and added test for transpose_square_swarp

…threads better.

…s for all functions in this file.

…ariables in BinaryHypercube. The current implementation asserts num_variables < 32 or 64 (platform-dependent).

utils.rs: Documentation

WizardOfMenlo · 2024-11-11T13:30:39Z

src/poly_utils/hypercube.rs

@@ -1,13 +1,24 @@
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
+
+// TODO (Gotti): Should pos rather be a u64? usize is platform-dependent, giving a platform-dependent limit on the number of variables.


Yes, I would guess that even u32 would be enough, but let's do u64 to be safe.

WizardOfMenlo · 2024-11-11T13:33:22Z

src/poly_utils/sequential_lag_poly.rs

@@ -4,12 +4,24 @@ use ark_ff::Field;

 use super::{hypercube::BinaryHypercubePoint, MultilinearPoint};

+/// There is an alternative (possibly more efficient) implementation that iterates over the x in Gray code ordering.


Yes, we had it implemented here: https://github.com/compsec-epfl/space-efficient-sumcheck/blob/main/src/provers/hypercube.rs
Not 100% sure if we are using this iterator anywhere at the moment.

WizardOfMenlo · 2024-11-11T13:34:52Z

src/utils.rs

@@ -16,22 +21,37 @@ pub fn to_binary(value: usize, n_bits: usize) -> Vec<bool> {
    result
 }

+// TODO(Gotti): n_bits is a misnomer if base > 2. Should be n_limbs or sth.
+// Also, should the behaviour for value >= base^n_bits be specified as part of the API or asserted not to happen?


Let's assert it out of existence ;)

WizardOfMenlo · 2024-11-11T13:35:42Z

src/utils.rs


    result
 }

+// Gotti: Consider renaming this function. The name sounds like it's a PRG.
+// TODO (Gotti): Check that ordering is actually correct at point of use (everything else is big-endian).


This randomness can actually be in whatever order we want, as long as it is self-consistent

WizardOfMenlo

LGTM!

GottfriedHerold and others added 26 commits September 23, 2024 17:13

utils.rs: Document is_power_of_two. Note this has a bug for n == 0.

2b0a946

utils.rs:

68b4748

- Document behaviour of to_binary (specify the endianness and what happens if value > 2**n_bits). - Add test for to_binary - Document base_decomposition

utils.rs: Comments

22a4d7c

Merge remote-tracking branch 'origin/main' into understanding_whir

a63f79e

ntt/matrix.rs: Improve documentation of some functions. Just make it …

f26d785

…more precise what they do, because there is some ambiguity from the name alone.

Renamed a variable (from up/down to upper/lower), which is more natur…

164e894

…al and avoids shadowing a variable name.

ntt/matrix.rs: make MatrixMut::swap unsafe (and change the sole calle…

8a9dc27

…r accordingly), because it lacks a bounds-check for the given indices.

ntt/matrix.rs: Remove bounds checks from MatrixMut::ptr_at and Matrix…

630114e

…Mut::ptr_at_mut. These bounds-check are repeated in the respective sole caller (which is not unsafe). Alternatively, we could remove declare ptr_at and ptr_at_mut as *not* unsafe.

utils.rs: Actually fix is_power_of_two(0) to returns false.

83fa9fb

minor documentation and style improvements.

5cc1727

ntt/utils.rs: added tests and improved documentation ntt/utils.rs: Changed lcm(a,b) slightly to avoid a potential overflow of an intermediate result. Now it overflows iff the result overflows.

ntt/transpose.rs: Wrote test for transpose_copy. The function has bee…

32dc6fb

…n split into two versions (parallel | non-parallel) rather than an inline #[cfg], because that makes it hard to write unit tests.

ntt/transpoers.rs: Split functions into parallel and non-parallel; re…

6d02ed0

…worked tests and added test for transpose_square_swarp

Minor cleanup

4187c06

Cleanup.

06ea9dc

ntt/transpose.rs: Balance the workload of the binary tree of spawned …

c33e292

…threads better.

ntt/transpose.rs: Cleanup and minor documentation changes. Wrote test…

13f7e93

…s for all functions in this file.

ntt/ntt.rs Add some documentation.

e74bf94

poly_utils/hypercube.rs: Add documentation; add check for number of v…

11f87ce

…ariables in BinaryHypercube. The current implementation asserts num_variables < 32 or 64 (platform-dependent).

Fix some documentation; specify endianness choices.

bc933e9

poly_utils/{mod.rs, hypercube.rs}: Document what the functions do.

0efa279

utils.rs: Documentation

Comment LagrangePolynomialIterator

f87eaba

Documentation

f795266

Documentation

11d1eba

sync

6cdbcc4

Documenation updates.

608d947

Merge remote-tracking branch 'upstream/main' into understanding_whir

02b3c9c

GottfriedHerold changed the title ~~This PR mostly add documentation to the codebase that I was writing while trying to understand the code.~~ Documentation and minor robustness changes Nov 4, 2024

WizardOfMenlo reviewed Nov 11, 2024

View reviewed changes

WizardOfMenlo approved these changes Nov 11, 2024

View reviewed changes

WizardOfMenlo merged commit 3c764b4 into WizardOfMenlo:main Nov 11, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation and minor robustness changes #14

Documentation and minor robustness changes #14

GottfriedHerold commented Nov 4, 2024 •

edited

Loading

WizardOfMenlo Nov 11, 2024

WizardOfMenlo Nov 11, 2024

WizardOfMenlo Nov 11, 2024

WizardOfMenlo Nov 11, 2024

WizardOfMenlo left a comment

		@@ -1,13 +1,24 @@
		#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]

		// TODO (Gotti): Should pos rather be a u64? usize is platform-dependent, giving a platform-dependent limit on the number of variables.

		@@ -4,12 +4,24 @@ use ark_ff::Field;

		use super::{hypercube::BinaryHypercubePoint, MultilinearPoint};

		/// There is an alternative (possibly more efficient) implementation that iterates over the x in Gray code ordering.

Documentation and minor robustness changes #14

Documentation and minor robustness changes #14

Conversation

GottfriedHerold commented Nov 4, 2024 • edited Loading

WizardOfMenlo Nov 11, 2024

Choose a reason for hiding this comment

WizardOfMenlo Nov 11, 2024

Choose a reason for hiding this comment

WizardOfMenlo Nov 11, 2024

Choose a reason for hiding this comment

WizardOfMenlo Nov 11, 2024

Choose a reason for hiding this comment

WizardOfMenlo left a comment

Choose a reason for hiding this comment

GottfriedHerold commented Nov 4, 2024 •

edited

Loading