SecureColumn

[spapinistarkware]

This change is 

[spapinistarkware]

Current dependencies on/for this PR:

• SecureColumn #402 👈
• dev

This stack of pull requests is managed by Graphite.

[alonh5]

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

---

src/core/air/evaluation.rs line 17 at r1 (raw file):

use crate::core::utils::IteratorMutExt;

// TODO(spapini): find a better place for this

How about backend/mod.rs?

---

src/core/air/evaluation.rs line 21 at r1 (raw file):

    pub cols: [Col<B, BaseField>; <SecureField as ExtensionOf<BaseField>>::EXTENSION_DEGREE],
}
impl SecureColumn<CPUBackend> {

Add blank lines between blocks.

---

src/core/air/evaluation.rs line 154 at r1 (raw file):

        let mut res_coeffs = SecureColumn::<CPUBackend>::zeros(1 << self.log_size());
        let res_log_size = self.log_size();
        assert_eq!(self.n_cols_per_size[0], 0);

Can you move this assertion to fn columns()?

Code quote:

assert_eq!(self.n_cols_per_size[0], 0);

---

src/core/air/evaluation.rs line 160 at r1 (raw file):

            .skip(1)
            .map(|(log_size, values)| {
                let x = SecureColumn {

Suggestion:

coeffs

---

src/core/air/evaluation.rs line 171 at r1 (raw file):

                    }),
                };
                assert_eq!(x.len(), 1 << res_log_size);

Extract to a variable.

Code quote:

1 << res_log_size

---

src/core/air/evaluation.rs line 171 at r1 (raw file):

                    }),
                };
                assert_eq!(x.len(), 1 << res_log_size);

Why is this assertion needed?

Code quote:

assert_eq!(x.len(), 1 << res_log_size);

---

src/core/air/evaluation.rs line 177 at r1 (raw file):

            .for_each(|(coeffs, n_cols)| {
                // Add poly.coeffs into coeffs, elementwise, inplace.
                assert_eq!(coeffs.len(), 1 << res_log_size);

And again the same assertion?

Code quote:

assert_eq!(coeffs.len(), 1 << res_log_size);

---

src/core/air/evaluation.rs line 181 at r1 (raw file):

                for i in 0..(1 << res_log_size) {
                    let res_coeff = res_coeffs.at(i) * multiplier + coeffs.at(i);
                    res_coeffs.set(i, res_coeff);

Wouldn't it be more efficient to calculate res_coeff on a variable and only set it at the end? Like it was previously.

Code quote:

                    let res_coeff = res_coeffs.at(i) * multiplier + coeffs.at(i);
                    res_coeffs.set(i, res_coeff);

---

src/core/backend/mod.rs line 30 at r1 (raw file):

pub type Col<B, F> = <B as FieldOps<F>>::Column;

pub trait Column<F>: Clone + Debug + Index<usize, Output = F> + FromIterator<F> {

Add a TODO to check if we still need Column to be generic over Field after GKR.

Suggestion:

pub trait Column<F: Field>

---

src/core/backend/cpu/mod.rs line 19 at r1 (raw file):

}

impl<F: Clone + Debug + Field> Column<F> for Vec<F> {

If you make the suggested change above you can remove this, right?

Code quote:

+ Field

---

src/core/fields/cm31.rs line 16 at r1 (raw file):

/// Represented as (a, b) of a + bi.
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct CM31(pub M31, pub M31);

Maybe?

Suggestion:

pub(crate) struct CM31(pub M31, pub M31);

[spapinistarkware]

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

---

src/core/air/evaluation.rs line 17 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

How about backend/mod.rs?

This struct isn't related to the backend. It's 4 Base field columns. It could have existed before the backend change, and be 4 Vec

---

src/core/air/evaluation.rs line 21 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Add blank lines between blocks.

Done.

---

src/core/air/evaluation.rs line 154 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you move this assertion to fn columns()?

Done.

---

src/core/air/evaluation.rs line 160 at r1 (raw file):

            .skip(1)
            .map(|(log_size, values)| {
                let x = SecureColumn {

Done.

---

src/core/air/evaluation.rs line 171 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Extract to a variable.

Done.

---

src/core/air/evaluation.rs line 177 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

And again the same assertion?

It was debugging. Sorry.

---

src/core/air/evaluation.rs line 181 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Wouldn't it be more efficient to calculate res_coeff on a variable and only set it at the end? Like it was previously.

It's not a fold. Not a single element aggregated.
Both before and now, I'maccumulating into each cell of res_coeffs.

---

src/core/backend/mod.rs line 30 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Add a TODO to check if we still need Column to be generic over Field after GKR.

Done.

---

src/core/backend/cpu/mod.rs line 19 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

If you make the suggested change above you can remove this, right?

You mean if it's only supporting BaseField? Right.

---

src/core/fields/cm31.rs line 16 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Maybe?

Library users might want to use it.

[alonh5]

Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @spapinistarkware)

---

src/core/air/evaluation.rs line 21 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

Can you do it in all of them?

---

src/core/air/evaluation.rs line 171 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

I don't see it.

---

src/core/backend/mod.rs line 30 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

Also add the suggestion? Column<F: Field>

---

src/core/backend/cpu/mod.rs line 19 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

You mean if it's only supporting BaseField? Right.

I meant if you bound F to Field in column.

[spapinistarkware]

Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @alonh5)

---

src/core/air/evaluation.rs line 21 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you do it in all of them?

Done.

---

src/core/air/evaluation.rs line 171 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I don't see it.

Done.

---

src/core/backend/mod.rs line 30 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Also add the suggestion? Column<F: Field>

Done.

---

src/core/backend/cpu/mod.rs line 19 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I meant if you bound F to Field in column.

No.

[alonh5]

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