Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

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

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

🐈

@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

@000wan
Copy link
Contributor Author

000wan commented Jan 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leolara leolara Awaiting requested review from leolara

At least 1 approving review is required to merge this pull request.

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