Allow different degrees in witgen #1460

Schaeff · 2024-06-20T15:54:48Z

Fixes #1494

use cbor for witness and constant files (moving polygon serialisation to the relevant backend)
add degree field on Symbol, inherited from the namespace degree
have each machine in witgen operate over its own degree
fail in the backend if we have many degrees

ast/src/analyzed/display.rs

ast/src/analyzed/mod.rs

…nt-degrees

executor/src/witgen/generator.rs

leonardoalt · 2024-06-26T09:00:53Z

tests failing

…nt-degrees

executor/src/witgen/machines/block_machine.rs

chriseth · 2024-07-02T13:05:56Z

ast/src/analyzed/display.rs

+                let mut namespace =
+                    AbsoluteSymbolPath::default().join(SymbolPath::from_str(name).unwrap());
+                let name = namespace.pop().unwrap();
+                if namespace != current_namespace {


Should we add an assertion that the degree is the same within a namespace?

chriseth · 2024-07-02T13:07:17Z

ast/src/analyzed/mod.rs

    }
+
+    /// Returns the set of all degrees in this [`Analyzed<T>`].


Symbols without explicit degree are ignored.

chriseth · 2024-07-02T13:07:41Z

ast/src/analyzed/mod.rs

@@ -45,10 +44,28 @@ pub struct Analyzed<T> {
 }

 impl<T> Analyzed<T> {
-    /// @returns the degree if any. Panics if there is none.
+    /// Returns the common degree in this [`Analyzed<T>`].


Suggested change

/// Returns the common degree in this [`Analyzed<T>`].

/// Returns the degree common among all symbols that have an explicit degree.

chriseth · 2024-07-02T13:10:28Z

backend/src/estark/bin_exporter.rs

+    }
+
+    for i in 0..degree {
+        for (_name, constant) in polys {


constant?

chriseth · 2024-07-02T13:11:51Z

backend/src/estark/mod.rs

@@ -65,18 +67,18 @@ fn create_stark_struct(degree: DegreeType, hash_type: &str) -> StarkStruct {
    }
 }

-type PatchedConstants<F> = Vec<(String, Vec<F>)>;
+type Constants<F> = Vec<(String, Vec<F>)>;


Is this typedef needed?

Yes, it was already there thanks to clippy

chriseth · 2024-07-02T13:12:23Z

backend/src/estark/mod.rs


        // Pre-process the PIL and fixed columns.
-        let (pil, patched_constants) = first_step_fixup(analyzed, fixed);
+        let (pil, constants) = first_step_fixup(analyzed, fixed);


If we rename that variable, why not fixed?

chriseth · 2024-07-02T13:22:08Z

executor/src/witgen/mod.rs

@@ -279,6 +286,34 @@ pub struct FixedData<'a, T: FieldElement> {
 }

 impl<'a, T: FieldElement> FixedData<'a, T> {
+    pub fn common_degree<'b>(&self, ids: impl IntoIterator<Item = &'b PolyID>) -> DegreeType {


Please document

executor/src/witgen/mod.rs

chriseth · 2024-07-02T13:42:14Z

pil-analyzer/src/pil_analyzer.rs

-            } else {
-                self.polynomial_degree = Some(namespace_degree);
-            }
+            self.polynomial_degree = Some(namespace_degree);
        }


I would set polynomial_degree to None here in the else case. Otherwise, the next namespace "inherits" the previous namespace's degree if it's not set.

chriseth · 2024-07-02T13:52:02Z

I cannot really comment this in-line, so:

I would remove the degree parameter from Condenser::new and change Condenser::set_namespace to Condenser::set_namespace_and_degree, which takes the namespace and degree from the definition and stores it as the current namespace and degree in Condenser::degree.

…nt-degrees

chriseth · 2024-07-03T13:30:44Z

pipeline/tests/pil.rs

@@ -306,6 +306,13 @@ fn naive_byte_decomposition_gl() {
    verify_pil(f, Default::default());
 }

+#[test]
+#[should_panic = "NoVariableDegreeAvailable"]


Does this at least run witgen?

Yes! The error is thrown by the backend.

chriseth · 2024-07-04T14:34:46Z

plonky3/src/circuit_builder.rs

-            .collect();
+        let degrees = self.analyzed.degrees();
+
+        let values = match degrees.len() {


could use match &degrees[..] { [degree] => { ... }, [] => { ... }, _ => unreachable!()}

Schaeff added 4 commits June 19, 2024 19:56

wip

db92a63

make tests pass

3285424

lint, fmt

9507915

fix empty degree tests

e9a7199

chriseth reviewed Jun 21, 2024

View reviewed changes

ast/src/analyzed/display.rs Outdated Show resolved Hide resolved

chriseth reviewed Jun 21, 2024

View reviewed changes

ast/src/analyzed/mod.rs Outdated Show resolved Hide resolved

Schaeff added 2 commits June 24, 2024 21:05

Merge branch 'main' of github.com:powdr-labs/powdr into allow-differe…

3231e2b

…nt-degrees

use RawPolyID

591a0d8

Schaeff commented Jun 25, 2024

View reviewed changes

executor/src/witgen/generator.rs Show resolved Hide resolved

Schaeff added 2 commits June 25, 2024 16:27

clean

c9e0ebd

fix tests

925611c

Schaeff added 14 commits June 26, 2024 12:18

fix estark artifacts

17da5ce

use cbor

adadec7

Merge branch 'main' of github.com:powdr-labs/powdr into allow-differe…

ac851ab

…nt-degrees

clippy

896a1dd

commit file.. fmt lint

9a23e31

clean

9420d05

fix polygon

0f6ca05

expect proving to fail

c3c1700

fix test

22fe520

fmt

625bf41

commit two_proof.pil

e7091f8

fix vec_median test

d3b8414

pull main

0ececa4

revert pil file

7ba9587

chriseth reviewed Jun 27, 2024

View reviewed changes

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

chriseth reviewed Jun 27, 2024

View reviewed changes

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

Schaeff added 2 commits June 27, 2024 13:58

fix btree test, pass correct degree to main machine generator

62a0af7

simplify, thanks chris

5bc0ec0

Schaeff added 2 commits July 1, 2024 20:23

fix lookup

a3dd473

fix test

109408b

chriseth reviewed Jul 2, 2024

View reviewed changes

backend/src/estark/bin_exporter.rs Outdated

}

for i in 0..degree {

for (_name, constant) in polys {

Copy link

Member

chriseth Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constant?

chriseth reviewed Jul 2, 2024

View reviewed changes

executor/src/witgen/mod.rs Show resolved Hide resolved

chriseth reviewed Jul 2, 2024

View reviewed changes

Schaeff added 4 commits July 2, 2024 19:31

address review comments

9977a13

Merge branch 'main' of github.com:powdr-labs/powdr into allow-differe…

a0ff0ee

…nt-degrees

fix condenser

ebb1561

Merge branch 'main' of github.com:powdr-labs/powdr into allow-differe…

1803f93

…nt-degrees

chriseth previously approved these changes Jul 3, 2024

View reviewed changes

chriseth added this pull request to the merge queue Jul 3, 2024

chriseth removed this pull request from the merge queue due to a manual request Jul 3, 2024

chriseth reviewed Jul 3, 2024

View reviewed changes

pull main

1048471

Schaeff dismissed chriseth’s stale review via 1048471 July 3, 2024 21:12

Schaeff added 2 commits July 4, 2024 00:39

pull main

4dbbe21

fix empty pil test in p3

3a0ccbe

chriseth reviewed Jul 4, 2024

View reviewed changes

chriseth approved these changes Jul 4, 2024

View reviewed changes

Schaeff added this pull request to the merge queue Jul 4, 2024

Merged via the queue into main with commit 49ffd47 Jul 4, 2024
6 checks passed

Schaeff deleted the allow-different-degrees branch July 4, 2024 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different degrees in witgen #1460

Allow different degrees in witgen #1460

Schaeff commented Jun 20, 2024 •

edited by georgwiese

Loading

leonardoalt commented Jun 26, 2024

chriseth Jul 2, 2024

chriseth Jul 2, 2024

chriseth Jul 2, 2024

chriseth Jul 2, 2024

chriseth Jul 2, 2024

Schaeff Jul 2, 2024

chriseth Jul 2, 2024

chriseth Jul 2, 2024

chriseth Jul 2, 2024 •

edited

Loading

chriseth commented Jul 2, 2024

chriseth Jul 3, 2024

Schaeff Jul 3, 2024

chriseth Jul 4, 2024 •

edited

Loading

		}

		/// Returns the set of all degrees in this [`Analyzed<T>`].

	/// Returns the common degree in this [`Analyzed<T>`].
	/// Returns the degree common among all symbols that have an explicit degree.

Allow different degrees in witgen #1460

Allow different degrees in witgen #1460

Conversation

Schaeff commented Jun 20, 2024 • edited by georgwiese Loading

leonardoalt commented Jun 26, 2024

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 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

chriseth commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Schaeff commented Jun 20, 2024 •

edited by georgwiese

Loading

chriseth Jul 2, 2024 •

edited

Loading

chriseth Jul 4, 2024 •

edited

Loading