CyclicEvaluation

[spapinistarkware]

This change is 

[spapinistarkware]

Current dependencies on/for this PR:

• Support ifft for itnermediate sizes #383
• AVX fft benchmark #382
• ifft_full #381
• Save twiddles #380
• ifft_lower #379
• avx ifft3 #378
• avx vecwise butterflies #377
• avx vecwise ibutterfly #368
• avx ibutterfly #367
• avx butterfly #366
• avx poly #365
• AVX backend #364
• AVX bit reverse #353
• Fri backend #352
• LinePoly backend #350
• Backend #345
• Rename pair_excluder to pair_vanishing #337
• Fibonacci component #332
• CyclicEvaluation #336 👈
• Accumulators #335
• PointEvaluationAccumulator #334
• dev

This stack of pull requests is managed by Graphite.

[alonh5]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @spapinistarkware)

---

src/lib.rs line 9 at r1 (raw file):

    stdsimd,
    get_many_mut,
    option_get_or_insert_default

Where is this needed?

Code quote:

option_get_or_insert_default

---

src/core/poly/circle.rs line 265 at r1 (raw file):

    pub fn fetch_eval_on_coset(&self, coset: Coset) -> CyclicSlice<'_, F> {
        assert!(coset.size() <= self.domain.half_coset.size());

You can save the calculations. Or does the compiler already do this?

Suggestion:

assert!(coset.log_size() <= self.domain.half_coset.log_size());

---

src/core/poly/circle.rs line 266 at r1 (raw file):

    pub fn fetch_eval_on_coset(&self, coset: Coset) -> CyclicSlice<'_, F> {
        assert!(coset.size() <= self.domain.half_coset.size());
        if let Some(d) = self.domain.half_coset.find(coset.initial_index) {

Suggestion:

Some(offset)

---

src/core/poly/circle.rs line 296 at r1 (raw file):

}

pub struct CyclicSlice<'a, F: ExtensionOf<BaseField>> {

Add a docstring please.

---

src/core/poly/circle.rs line 306 at r1 (raw file):

    fn index(&self, index: isize) -> &Self::Output {
        let index = ((self.offset as isize) + index * (self.step as isize))
            & ((self.evaluation.len() - 1) as isize);

Is anything enforcing that self.evaluation's size is a power of 2?

Code quote:

& ((self.evaluation.len() - 1) as isize);

---

src/core/poly/circle.rs line 672 at r1 (raw file):

        let values = (0..domain.size()).map(|i| m31!(i as u32)).collect();
        let circle_evaluation = CircleEvaluation::<BaseField>::new(domain, values);
        let coset = Coset::new(CirclePointIndex::subgroup_gen(8).mul(7), 3);

Suggestion:
(It's clearer why the coset is contained in the domain)

Suggestion:

let coset = Coset::new(domain.index_at(17), 3);

[spapinistarkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5)

---

src/core/poly/circle.rs line 296 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Add a docstring please.

Done.

---

src/core/poly/circle.rs line 306 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Is anything enforcing that self.evaluation's size is a power of 2?

Yes. It is aprivate, and can only be created by fetch_eval_on_coset.

---

src/core/poly/circle.rs line 672 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Suggestion:
(It's clearer why the coset is contained in the domain)

Done.

[alonh5]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

src/core/poly/circle.rs line 306 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Yes. It is aprivate, and can only be created by fetch_eval_on_coset.

Do you think we should add a check so CosetSubEvaluation isn't used incorrectly (if for example someone wants to create one in a different way)?

[spapinistarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5)

---

src/core/poly/circle.rs line 306 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Do you think we should add a check so CosetSubEvaluation isn't used incorrectly (if for example someone wants to create one in a different way)?

It cannot be instantiated outside of this module, the fields are private. The only way to create it right now is from the above struct. It's essentially an inner struct.

Anyway, added validaiton on new().

[alonh5]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @spapinistarkware)

---

src/core/poly/circle.rs line 677 at r3 (raw file):

    #[test]
    fn test_sub_evaluartion() {

Suggestion:

fn test_sub_evaluation() {

---

src/core/poly/circle.rs line 682 at r3 (raw file):

        let circle_evaluation = CircleEvaluation::<BaseField>::new(domain, values);
        let coset = Coset::new(domain.index_at(17), 3);
        let slice = circle_evaluation.fetch_eval_on_coset(coset);

Suggestion:

sub_eval

[alonh5]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

[alonh5]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

src/core/poly/circle.rs line 301 at r4 (raw file):

    offset: usize,
    step: usize,
}

Add blank lines between code blocks.

The base branch was changed.

[alonh5]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)