Add PreProcessedColumn Trait

[Gali-StarkWare]

No description provided.

[reviewable-StarkWare]

This change is 

[Gali-StarkWare]

• Remove PreProcessedColumn Enum #968
• Use PreProcessedColumn Trait Instead of Enum #967
• Add Structs using PreprocessedColumn Trait #966
• Add PreProcessedColumn Trait #965 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.23%. Comparing base (31e8dbc) to head (c7e7607).

Files with missing lines	Patch %	Lines
...r/src/constraint_framework/preprocessed_columns.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #965      +/-   ##
==========================================
- Coverage   92.26%   92.23%   -0.04%     
==========================================
  Files         105      105              
  Lines       14293    14299       +6     
  Branches    14293    14299       +6     
==========================================
  Hits        13188    13188              
- Misses       1032     1038       +6     
  Partials       73       73              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 24 at r1 (raw file):

pub trait PreprocessedColumnTrait: Debug {
    fn name(&self) -> &'static str;
    fn id(&self) -> String;

doc what is this used for, and what should the implementor be aware of

Code quote:

fn id(&self) -> String;

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 29 at r1 (raw file):

    fn packed_at(&self, vec_row: usize) -> PackedM31;
    // TODO: CPU logic.
    /// Generates a column according to the preprocessed column chosen.

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 38 at r1 (raw file):

}
impl Eq for dyn PreprocessedColumnTrait {}
impl Hash for dyn PreprocessedColumnTrait {

maybe hash is better off handled by the struct itself

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 22 at r1 (raw file):

// TODO(Gali): Rename to PrerocessedColumn.
pub trait PreprocessedColumnTrait: Debug {

I think this trait hides 2 separate functionalities within it,
one is a mapping from column to it's id, which is used primarily in the constraints part,
the other is tools for trace gen, which technically is possible to live without, and the place for those is not in this trait, or in the constraint framework at all (I know they are here now).
I'm asking myself (and you), why are they behind a trait? when you hold the column during tracegen, you know the concrete type, and can use it's methods without dyn or generics.
I'm suggesting leaving only name, id,logsize here, the packed at and gen can stay behind the impl of the concrete structs (which is the case currently).

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 22 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

I think this trait hides 2 separate functionalities within it,
one is a mapping from column to it's id, which is used primarily in the constraints part,
the other is tools for trace gen, which technically is possible to live without, and the place for those is not in this trait, or in the constraint framework at all (I know they are here now).
I'm asking myself (and you), why are they behind a trait? when you hold the column during tracegen, you know the concrete type, and can use it's methods without dyn or generics.
I'm suggesting leaving only name, id,logsize here, the packed at and gen can stay behind the impl of the concrete structs (which is the case currently).

Done. I'll also move the structs to the examples later in the stack

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 24 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

doc what is this used for, and what should the implementor be aware of

Done.

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 38 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

maybe hash is better off handled by the struct itself

Not sure this will work, will try and update here

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 29 at r1 (raw file):

    fn packed_at(&self, vec_row: usize) -> PackedM31;
    // TODO: CPU logic.
    /// Generates a column according to the preprocessed column chosen.

Done.

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 22 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Done. I'll also move the structs to the examples later in the stack

Now I face a problem with gen_preprocessed_columns because it needs gen_column_simd() (in prove_state_machine)

[ohad-starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 22 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Now I face a problem with gen_preprocessed_columns because it needs gen_column_simd() (in prove_state_machine)

change it

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 25 at r2 (raw file):

    /// Used for hashing and comparison of preprocessed columns.
    /// The id should be unique for each preprocessed column - one naive
    /// implementation is: "PreProcessedColumnName(PreProcessedColumnsVariables)".

Suggestion:

    /// Used to compare preprocessed columns.
    /// Column IDs must be unique in a given context. 

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 22 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

change it

Done.

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 25 at r2 (raw file):

    /// Used for hashing and comparison of preprocessed columns.
    /// The id should be unique for each preprocessed column - one naive
    /// implementation is: "PreProcessedColumnName(PreProcessedColumnsVariables)".

Done.

[ohad-starkware]

crates/prover/src/constraint_framework/preprocessed_columns.rs line 38 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Not sure this will work, will try and update here

did you try?

[Gali-StarkWare]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/prover/src/constraint_framework/preprocessed_columns.rs line 38 at r1 (raw file):

Previously, ohad-starkware (Ohad) wrote…

did you try?

Yes, it cannot live together with the dyn trait

[ohad-starkware]

crates/prover/src/constraint_framework/preprocessed_columns.rs line 38 at r1 (raw file):

Previously, Gali-StarkWare wrote…

Yes, it cannot live together with the dyn trait

let's try to stop using hashmap and then you wont need it