Witgen for multiplicities in LogUp in PIL #1686

onurinanc · 2024-08-14T14:22:53Z

Opened this PR to discuss on the change files related to the issue #1573

columns: {"main.m_logup_multiplicity": [Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }]}
Vec<T> parts: [[Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }]]
String parts: ["main.m_logup_multiplicity"]
columns.len(): 1
witness_cols: [("main.y", [Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([6, 0, 0, 0]) }, Bn254Field { value: BigInt([3, 0, 0, 0]) }, Bn254Field { value: BigInt([7, 0, 0, 0]) }, Bn254Field { value: BigInt([5, 0, 0, 0]) }, Bn254Field { value: BigInt([3, 0, 0, 0]) }, Bn254Field { value: BigInt([7, 0, 0, 0]) }, Bn254Field { value: BigInt([4, 0, 0, 0]) }]), ("main.m_logup_multiplicity", [Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }])]

Using take_witness_col_values inside the FixedLookup machine, we have the correct values of the main.m_logup_multiplicitiy column, However, when it comes to witness_cols, the values are changed.
The above issue is fixed.

With this PR, we have a witness generation for multiplicities in LogUp / bus argument.

executor/src/witgen/machines/fixed_lookup_machine.rs

executor/src/witgen/mod.rs

Schaeff · 2024-08-19T17:00:56Z

executor/src/witgen/machines/fixed_lookup_machine.rs

+            .collect()
+    }
+
+    fn get_namespace(&self) -> String {


This seems to me like it still has the same issue: self.fixed_data.witness_cols contains all witness columns in the pil, across all namespaces. So what this function does is rather get_first_namespace which I doubt would work if we had for example two machines with each a col witness m_logup_multiplicity. The same goes for the has_logup_multiplicity_column.

I would suggest adding a pipeline test which does witgen for two machines with each a multiplicity column.

You're right. For example, should it work for both "main.m_logup_multiplicity" and "arith.m_logup_multiplicity"? Or, should we collect all the namespaces and return None from try_new by adding the following as same as the double_sorted_witness_machine?

if namespaces.len() > 1 { // columns are not in the same namespace, fail return None; }

Also, the function take_witness_col_values() in fixed_lookup_machine should return both main.m_logup_multiplicity and arith.m_logup_multiplicity right?

What do you think about adding if namespaces.len() > 1 { // columns are not in the same namespace, fail return None; } mechanism in this PR, and adding an example with 2 different namespaces together with a witgen fix in the next PR?

Yeah, since the FixedLookup currently is a strange machine in that it is responsible for columns of different namespaces, it would be responsible for all multiplicity columns associated with any lookup in connecting_identities. We could store a multiplicity_columns: BTreeMap<u64, PolyID> (identity ID -> multiplicity poly ID).

Figuring out this mapping is a bit of a hassle and obsolete after #1378 anyway though, so I think it could be fair to defer to a different PR. But I think failing in the presence of > 1 namespace is overly strict, we could fail if there is > 1 column called *.m_logup_multiplicity, right?

Maybe instead of has_logup_multiplicity_column, you can store logup_multiplicity_column: Option<PolyID>. Then, instead of calling self.namespaced(MULTIPLICITY_LOOKUP_COLUMN), you can do logup_multiplicity_column_name = logup_multiplicity_column.map(|poly_id| self.fixed_data.column_name(poly_id)).

georgwiese · 2024-08-20T08:22:41Z

executor/src/witgen/machines/fixed_lookup_machine.rs

+            .collect()
+    }
+
+    fn get_namespace(&self) -> String {


Yeah, since the FixedLookup currently is a strange machine in that it is responsible for columns of different namespaces, it would be responsible for all multiplicity columns associated with any lookup in connecting_identities. We could store a multiplicity_columns: BTreeMap<u64, PolyID> (identity ID -> multiplicity poly ID).

Figuring out this mapping is a bit of a hassle and obsolete after #1378 anyway though, so I think it could be fair to defer to a different PR. But I think failing in the presence of > 1 namespace is overly strict, we could fail if there is > 1 column called *.m_logup_multiplicity, right?

Maybe instead of has_logup_multiplicity_column, you can store logup_multiplicity_column: Option<PolyID>. Then, instead of calling self.namespaced(MULTIPLICITY_LOOKUP_COLUMN), you can do logup_multiplicity_column_name = logup_multiplicity_column.map(|poly_id| self.fixed_data.column_name(poly_id)).

executor/src/witgen/machines/fixed_lookup_machine.rs

georgwiese · 2024-08-20T08:38:22Z

executor/src/witgen/machines/fixed_lookup_machine.rs

+        // Clones the self.multiplcities by changing the type of the multiplicity from u64 to T by using T::from()
+        // and adds the rows that are not present in the multiplicities for each identity as T::zero()
+        let multiplicities: BTreeMap<u64, BTreeMap<usize, T>> = self
+            .multiplicities
+            .clone()
+            .into_iter()
+            .map(|(identity_id, multiplicity)| {
+                let mut multiplicity: BTreeMap<usize, T> = multiplicity
+                    .into_iter()
+                    .map(|(row, multiplicity)| (row, T::from(multiplicity)))
+                    .collect();
+                for row in 0..self.degree as usize {
+                    multiplicity.entry(row).or_insert_with(|| T::zero());
+                }
+                (identity_id, multiplicity)
+            })
+            .collect();
+
+        // Collects all the rows of an identity as a Vec<T>
+        let mut witness_col_values = HashMap::new();
+        for multiplicity in multiplicities.values() {
+            let mut values = vec![];
+            for row in 0..self.degree as usize {
+                values.push(multiplicity[&row]);
+            }
+            if self.has_logup_multiplicity_column {
+                log::trace!("Detected LogUp Multiplicity Column");
+                witness_col_values.insert(self.namespaced(MULTIPLICITY_LOOKUP_COLUMN), values);
+            }
+        }
+        witness_col_values


Suggested change

// Clones the self.multiplcities by changing the type of the multiplicity from u64 to T by using T::from()

// and adds the rows that are not present in the multiplicities for each identity as T::zero()

let multiplicities: BTreeMap<u64, BTreeMap<usize, T>> = self

.multiplicities

.clone()

.into_iter()

.map(|(identity_id, multiplicity)| {

let mut multiplicity: BTreeMap<usize, T> = multiplicity

.into_iter()

.map(|(row, multiplicity)| (row, T::from(multiplicity)))

.collect();

for row in 0..self.degree as usize {

multiplicity.entry(row).or_insert_with(|| T::zero());

}

(identity_id, multiplicity)

})

.collect();

// Collects all the rows of an identity as a Vec<T>

let mut witness_col_values = HashMap::new();

for multiplicity in multiplicities.values() {

let mut values = vec![];

for row in 0..self.degree as usize {

values.push(multiplicity[&row]);

}

if self.has_logup_multiplicity_column {

log::trace!("Detected LogUp Multiplicity Column");

witness_col_values.insert(self.namespaced(MULTIPLICITY_LOOKUP_COLUMN), values);

}

}

witness_col_values

let mut witness_col_values = HashMap::new();

if self.has_logup_multiplicity_column {

assert!(self.multiplicities.len() <= 1, "LogUp witness generation not yet supported for > 1 lookups");

log::trace!("Detected LogUp Multiplicity Column");

for multiplicity in std::mem::take(&mut self.multiplicities).into_values() {

let mut values = vec![];

for row in 0..self.degree as usize {

values.push(T::from(multiplicity.get(&row).cloned().unwrap_or_default()));

}

witness_col_values.insert(self.namespaced(MULTIPLICITY_LOOKUP_COLUMN), values);

}

}

witness_col_values

How about this? I don't think there is a need to build a Map with a big number of 0s first.

Yes, this looks better

georgwiese

Cool, think this is good for a first version. We should should implement #1378 soon, so that we can get rid of the pattern matching and support more than one lookup.

I think the test needs to be adjusted after #1802 is merged (currently in queue), and after that can actually be tested with Goldilocks as well and no longer needs to be blacklisted in the parsing test.

executor/src/witgen/machines/fixed_lookup_machine.rs

chriseth · 2024-09-23T15:14:06Z

executor/src/witgen/machines/fixed_lookup_machine.rs

@@ -265,6 +307,14 @@ impl<'a, T: FieldElement> FixedLookup<'a, T> {
            }
        };

+        // Update the multiplicities
+        self.multiplicities


This also stores the value if we don't have/need a multiplicities column, right?

Please only do this if we have a multiplicities column.

chriseth · 2024-09-23T15:15:24Z

executor/src/witgen/machines/fixed_lookup_machine.rs

+
+        // This currenlty just takes one element with the correct name
+        // When we support more than one element, we need to have a vector of logup_multiplicity_columns: Vec<Option<PolyId>>
+        let logup_multiplicity_column: Option<PolyID> = fixed_data


Shouldn't this use a different column for each lookup (based on the lookup ID)?

Yes, but figuring out that mapping is not straight-forward (would need to find and analyze the polynomial identities belonging to the lookup), whereas it's trivial after #1378 (because we'll have an explicit annotation). So, in my opinion, we should get that done first.

chriseth · 2024-09-30T12:51:45Z

pipeline/tests/powdr_std.rs

        "std/bus_permutation_via_challenges.asm",
        "std/permutation_via_challenges.asm",
        "std/lookup_via_challenges.asm",
        "std/poseidon_bn254_test.asm",
        "std/split_bn254_test.asm",
        "std/bus_lookup_via_challenges.asm",
+        "std/multiplicities.asm",


Is this correct? Doesn't it automatically use the extension field now?

chriseth · 2024-09-30T12:52:17Z

executor/src/witgen/mod.rs

@@ -262,7 +262,7 @@ impl<'a, 'b, T: FieldElement> WitnessGenerator<'a, 'b, T> {
            generator
        });

-        // Get columns from machines
+        // Get the columns from machines


I would say either get columns from machines or get the columns from the machines

chriseth · 2024-10-15T11:58:38Z

executor/src/witgen/machines/fixed_lookup_machine.rs

+        let logup_multiplicity_column: Option<PolyID> = fixed_data
+            .witness_cols
+            .values()
+            .find(|col| split_column_name(&col.poly.name).1 == MULTIPLICITY_LOOKUP_COLUMN)


Suggested change

.find(|col| split_column_name(&col.poly.name).1 == MULTIPLICITY_LOOKUP_COLUMN)

.find(|col| SymbolPath::from_str(&col.poly.name).name() == MULTIPLICITY_LOOKUP_COLUMN)

chriseth · 2024-10-15T12:03:15Z

executor/src/witgen/machines/fixed_lookup_machine.rs

+            .multiplicities
+            .entry(identity_id)
+            .or_default()
+            .entry(row)


In the end, we create a full vector of the full column and store it in memory. Doesn't it make sense to start with a vector (instead of a hashmap) to begin with?

But, then I need to change this part

chriseth · 2024-10-15T15:27:57Z

executor/src/witgen/machines/fixed_lookup_machine.rs

@@ -185,7 +180,7 @@ pub struct FixedLookup<'a, T: FieldElement> {
    indices: IndexedColumns<T>,
    connecting_identities: BTreeMap<u64, &'a Identity<T>>,
    fixed_data: &'a FixedData<'a, T>,
-    multiplicities: BTreeMap<u64, BTreeMap<usize, T>>,
+    multiplicities: BTreeMap<u64, Vec<T>>,


I already forgot what the key is, can you document it?

The key is identity_id. Do you want me to add as a comment?

Yes, please. Otherwise the PR is ok to go!

Ah and please also only store the multiplicity if we have a multiplicity column.

chriseth · 2024-10-15T15:45:43Z

executor/src/witgen/machines/fixed_lookup_machine.rs

-            .entry(identity_id)
-            .or_insert_with(|| vec![T::zero(); self.degree as usize])[row] += T::one();
+        // Update the multiplicities
+        if let Some(_) = self.logup_multiplicity_column {


Suggested change

if let Some(_) = self.logup_multiplicity_column {

if self.logup_multiplicity_column.is_some() {

chriseth · 2024-10-16T08:18:58Z

executor/src/witgen/machines/fixed_lookup_machine.rs

+/// `multiplicities` is a mapping between `identity_id` (u64) and a vector of multiplicities (Vec<T>).
 pub struct FixedLookup<'a, T: FieldElement> {
+    degree: DegreeType,
    global_constraints: GlobalConstraints<T>,
    indices: IndexedColumns<T>,
    connecting_identities: BTreeMap<u64, &'a Identity<T>>,
    fixed_data: &'a FixedData<'a, T>,
+    multiplicities: BTreeMap<u64, Vec<T>>,


Suggested change

/// `multiplicities` is a mapping between `identity_id` (u64) and a vector of multiplicities (Vec<T>).

pub struct FixedLookup<'a, T: FieldElement> {

degree: DegreeType,

global_constraints: GlobalConstraints<T>,

indices: IndexedColumns<T>,

connecting_identities: BTreeMap<u64, &'a Identity<T>>,

fixed_data: &'a FixedData<'a, T>,

multiplicities: BTreeMap<u64, Vec<T>>,

pub struct FixedLookup<'a, T: FieldElement> {

degree: DegreeType,

global_constraints: GlobalConstraints<T>,

indices: IndexedColumns<T>,

connecting_identities: BTreeMap<u64, &'a Identity<T>>,

fixed_data: &'a FixedData<'a, T>,

/// multiplicities column values for each identity id

multiplicities: BTreeMap<u64, Vec<T>>,

Judging from the build failures of #1686 our requests to https://opensource.org/license/MIT are being blocked. Ignore the license URLs when checking the links.

chriseth · 2024-10-16T13:53:26Z

I think there is some kind of merge problem here.

onurinanc · 2024-10-17T06:55:28Z

I think there is some kind of merge problem here.

resolved

onurinanc · 2024-10-17T09:32:09Z

I think there is some kind of merge problem here.

resolved

@chriseth?

Opened this PR to discuss on the change files related to the issue #1573 ``` columns: {"main.m_logup_multiplicity": [Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }]} Vec<T> parts: [[Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([1, 0, 0, 0]) }, Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }]] String parts: ["main.m_logup_multiplicity"] columns.len(): 1 witness_cols: [("main.y", [Bn254Field { value: BigInt([2, 0, 0, 0]) }, Bn254Field { value: BigInt([6, 0, 0, 0]) }, Bn254Field { value: BigInt([3, 0, 0, 0]) }, Bn254Field { value: BigInt([7, 0, 0, 0]) }, Bn254Field { value: BigInt([5, 0, 0, 0]) }, Bn254Field { value: BigInt([3, 0, 0, 0]) }, Bn254Field { value: BigInt([7, 0, 0, 0]) }, Bn254Field { value: BigInt([4, 0, 0, 0]) }]), ("main.m_logup_multiplicity", [Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }, Bn254Field { value: BigInt([0, 0, 0, 0]) }])] ``` Using take_witness_col_values inside the FixedLookup machine, we have the correct values of the `main.m_logup_multiplicitiy` column, However, when it comes to witness_cols, the values are changed. The above issue is fixed. With this PR, we have a witness generation for multiplicities in LogUp / bus argument.

Schaeff reviewed Aug 16, 2024

View reviewed changes

executor/src/witgen/machines/fixed_lookup_machine.rs Outdated Show resolved Hide resolved

Schaeff reviewed Aug 19, 2024

View reviewed changes

georgwiese reviewed Aug 20, 2024

View reviewed changes

onurinanc requested a review from georgwiese August 27, 2024 09:30

onurinanc changed the title ~~[WIP] Witgen for multiplicities in LogUp in PIL~~ Witgen for multiplicities in LogUp in PIL Sep 23, 2024

georgwiese reviewed Sep 23, 2024

View reviewed changes

executor/src/witgen/machines/fixed_lookup_machine.rs Outdated Show resolved Hide resolved

chriseth reviewed Sep 23, 2024

View reviewed changes

onurinanc added 17 commits September 27, 2024 12:34

change fixed_lookup_machine

dc023b9

remove multiplicity column from base witnesses

e8e4cb3

remove .vscode/

267de25

fix linter tests

e7344f7

format

a7cf1dc

remove wrong example

4371ea3

add multiplicities to reparse blacklist

3164579

remove multiplicities from blacklist and fix test

8bff576

refactor fixed lookup struct & add multiplicities to the blacklist

9789aba

cargo fmt

230f591

fix blacklist

decbd57

remove two lookup ex

65ea43f

easy fixes

abcc942

remove namespaced, has_logup_multiplicity, and unnecessary adding of 0s

e6a37ea

fix test

1c6aea5

fix multiplicities for the new lookup

4caefed

fix split_column_name separator

7b2a8f4

onurinanc force-pushed the witgen-multiplicities-for-logup branch from fd53228 to 7b2a8f4 Compare September 27, 2024 10:42

onurinanc requested review from georgwiese and chriseth September 27, 2024 11:11

chriseth reviewed Sep 30, 2024

View reviewed changes

onurinanc requested review from Schaeff and chriseth October 15, 2024 11:46

chriseth reviewed Oct 15, 2024

View reviewed changes

onurinanc added 3 commits October 15, 2024 15:30

change BTreeMap to Vec

9feda43

fix vec

b8261ff

fix update method

0eb6cdb

chriseth reviewed Oct 15, 2024

View reviewed changes

onurinanc added 2 commits October 15, 2024 17:34

add documentation for multiplicities

17dd181

add conditional to store multiplicities

385c8ae

chriseth reviewed Oct 15, 2024

View reviewed changes

fix

95d23a3

onurinanc requested a review from chriseth October 15, 2024 16:26

chriseth reviewed Oct 16, 2024

View reviewed changes

fix comment

7ee5438

Schaeff mentioned this pull request Oct 16, 2024

Ignore licenses in link check #1912

Merged

github-merge-queue bot pushed a commit that referenced this pull request Oct 16, 2024

Ignore licenses in link check (#1912)

93770dd

Judging from the build failures of #1686 our requests to https://opensource.org/license/MIT are being blocked. Ignore the license URLs when checking the links.

onurinanc added 2 commits October 16, 2024 13:04

markdown

cc9e51e

pull

a27cf39

resolve conflicts

6763547

onurinanc requested a review from chriseth October 17, 2024 06:55

chriseth approved these changes Oct 17, 2024

View reviewed changes

chriseth added this pull request to the merge queue Oct 17, 2024

Merged via the queue into main with commit 4721b47 Oct 17, 2024
14 checks passed

chriseth deleted the witgen-multiplicities-for-logup branch October 17, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Witgen for multiplicities in LogUp in PIL #1686

Witgen for multiplicities in LogUp in PIL #1686

onurinanc commented Aug 14, 2024 •

edited

Loading

Schaeff Aug 19, 2024

onurinanc Aug 19, 2024

onurinanc Aug 19, 2024

onurinanc Aug 19, 2024

georgwiese Aug 20, 2024

georgwiese Aug 20, 2024

georgwiese Aug 20, 2024

onurinanc Aug 22, 2024

georgwiese left a comment

chriseth Sep 23, 2024

chriseth Oct 15, 2024

chriseth Sep 23, 2024

georgwiese Sep 23, 2024

chriseth Sep 30, 2024

onurinanc Oct 15, 2024

chriseth Sep 30, 2024

chriseth Oct 15, 2024

chriseth Oct 15, 2024

onurinanc Oct 15, 2024

chriseth Oct 15, 2024 •

edited

Loading

onurinanc Oct 15, 2024

onurinanc Oct 15, 2024

chriseth Oct 15, 2024

chriseth Oct 15, 2024

chriseth Oct 15, 2024

onurinanc Oct 15, 2024

chriseth Oct 16, 2024

chriseth commented Oct 16, 2024

onurinanc commented Oct 17, 2024

onurinanc commented Oct 17, 2024

	.find(\|col\| split_column_name(&col.poly.name).1 == MULTIPLICITY_LOOKUP_COLUMN)
	.find(\|col\| SymbolPath::from_str(&col.poly.name).name() == MULTIPLICITY_LOOKUP_COLUMN)

	if let Some(_) = self.logup_multiplicity_column {
	if self.logup_multiplicity_column.is_some() {

Witgen for multiplicities in LogUp in PIL #1686

Witgen for multiplicities in LogUp in PIL #1686

Conversation

onurinanc commented Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgwiese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Oct 16, 2024

onurinanc commented Oct 17, 2024

onurinanc commented Oct 17, 2024

onurinanc commented Aug 14, 2024 •

edited

Loading

chriseth Oct 15, 2024 •

edited

Loading