Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

Don't add column for SSB with only one step type #134

Closed
wants to merge 6 commits into from
Closed

Don't add column for SSB with only one step type #134

wants to merge 6 commits into from

Conversation

000wan
Copy link
Contributor

@000wan 000wan commented Sep 18, 2023

Summary

It resolves #95.

It makes SimpleStepSelectorBuilder not add any step selector columns if there is only one step.
Further, I wrote a unit test for this (SSB with single step) and it might be helpful for #102.

Test Results

Passes all tests with cargo test.
Runs Ok with /examples/fibonacci.rs, /examples/mimc7.rs, /examples/poseidon.rs
Witness display results of cargo run --package chiquito --example fibonacci changed:

Before:

offset,srcm forward a,srcm forward b,srcm internal signal c,'step selector for fibo step'
0,1,1,2,1
1,1,2,3,1
2,2,3,5,1
3,3,5,8,1
4,5,8,13,1
5,8,13,21,1
6,13,21,34,1
7,21,34,55,1
8,34,55,89,1
9,55,89,144,1
10,89,144,233,1

After:

offset,srcm forward a,srcm forward b,srcm internal signal c
0,1,1,2
1,1,2,3
2,2,3,5
3,3,5,8
4,5,8,13
5,8,13,21
6,13,21,34
7,21,34,55
8,34,55,89
9,55,89,144
10,89,144,233

@000wan
Copy link
Contributor Author

000wan commented Sep 18, 2023 •
edited
Loading

Ready for review @leolara

leolara
leolara reviewed Sep 22, 2023
View reviewed changes
src/plonkish/compiler/step_selector.rs Outdated

selector
.selector_assignment
.insert(step.uuid(), vec![(PolyExpr::Const(F::ONE), F::ONE)]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@000wan this looks not the more readable way. I would make get_selector_assignment to return an Option, and then the code in assignment generator, could ignore this if None is returned.

Copy link
Contributor Author

@000wan 000wan Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with that. Now I modified selector_assignment to have an Option value.

@000wan 000wan requested a review from leolara September 22, 2023 13:01
@000wan 000wan changed the title SimpleStepSelectorBuilder for only one step type Don't add column for SSB with only one step type Sep 25, 2023
@000wan
Copy link
Contributor Author

000wan commented Sep 27, 2023 •
edited
Loading

🐈

@000wan 000wan marked this pull request as draft September 28, 2023 19:21
@000wan 000wan marked this pull request as ready for review September 28, 2023 19:21
000wan
000wan commented Sep 28, 2023
View reviewed changes
src/plonkish/compiler/step_selector.rs
Comment on lines +80 to +98
// don't add a column for a single step type
if unit.step_types.len() == 1 {
let step = unit.step_types.values().next().expect("step not found");

// use F::ONE for selector constantly on, F::ZERO for off
selector
.selector_expr
.insert(step.uuid(), PolyExpr::Const(F::ONE));

selector
.selector_expr_not
.insert(step.uuid(), PolyExpr::Const(F::ZERO));

selector.selector_assignment.insert(step.uuid(), None);

unit.selector = selector;
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made StepSelector to have a constant ONE for selector expression so that it selects always the same step. And it seems it's somehow working... Do you think this is an acceptable solution? @leolara

src/plonkish/compiler/step_selector.rs
@@ -16,7 +16,7 @@ pub type SelectorAssignment<F> = (PolyExpr<F>, F);
pub struct StepSelector<F> {
pub selector_expr: HashMap<StepTypeUUID, PolyExpr<F>>,
pub selector_expr_not: HashMap<StepTypeUUID, PolyExpr<F>>,
pub selector_assignment: HashMap<StepTypeUUID, Vec<SelectorAssignment<F>>>,
pub selector_assignment: HashMap<StepTypeUUID, Option<Vec<SelectorAssignment<F>>>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this Option in order to distinguish the cases where the uuid is invalid, or the uuid is valid but has no assignments.

@000wan 000wan closed this Oct 15, 2023
@leolara leolara reopened this Oct 15, 2023
@leolara
Copy link
Collaborator

leolara commented Oct 15, 2023

@000wan we are in a hacker house mentoring, that is why we have not answered in so long. This PR will get merged for sure. Let me get back to you

@000wan
Copy link
Contributor Author

000wan commented Oct 15, 2023

I didn't know you were busy.. then I'll wait for it

@leolara
Copy link
Collaborator

leolara commented Nov 18, 2023

@000wan so the only problem now is that if the step has more than one row, then it will be active also for the rows that is not the first row of the step. The best way to solve this is to make q_enable to be only one at the beginning of each step. this change of course, only if the number of steps is one.

So currently q_enable is 1 for all rows. if we have only one step with row height 3, then q_enable should be 1,0,0,1,0,0,1,0,0,.. and so on

@leolara leolara force-pushed the main branch 4 times, most recently from 03ddbb5 to 8397503 Compare November 19, 2023 05:49
@000wan
Copy link
Contributor Author

000wan commented Jan 11, 2024

Then doesn't it require a new column for the step?

@leolara leolara closed this Jul 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@leolara leolara Awaiting requested review from leolara

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SimpleStepSelectorBuilder case when there is only one step
2 participants
@000wan @leolara