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

Make MaxWidthCellManager work with fixed signals - Issue #97 #135

Closed
wants to merge 7 commits into from
Closed

Make MaxWidthCellManager work with fixed signals - Issue #97 #135

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
218a047
where is max_width_fixed?
atsupontio Sep 18, 2023
87503ea
where is max_width_fixed? #97
atsupontio Sep 18, 2023
dd602f0
add max_width_fixed #97
atsupontio Sep 19, 2023
1d4f872
fix poseidon example #97
atsupontio Sep 19, 2023
dbc8cc8
reflect the content of the review #97
atsupontio Sep 20, 2023
8479b41
fix column position and poseidon example #97
atsupontio Sep 25, 2023
85d26f5
Merge branch 'main' into add_fixed_signals_#97
leolara Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ serde_json = "1.0"
[patch."https://github.com/privacy-scaling-explorations/halo2.git"]
halo2_proofs = { git = "https://github.com/appliedzkp/halo2.git", rev = "d3746109d7d38be53afc8ddae8fdfaf1f02ad1d7", features = [
"circuit-params",
] }
] }
13 changes: 5 additions & 8 deletions examples/poseidon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,25 +661,22 @@ fn poseidon_super_circuit<F: PrimeField + Eq + Hash>(
lens: Lens,
) -> SuperCircuit<F, ValuesAndLens<F>> {
super_circuit::<F, ValuesAndLens<F>, _>("poseidon", |ctx| {
let single_config = config(SingleRowCellManager {}, SimpleStepSelectorBuilder {});
let config = config(MaxWidthCellManager::new(2, 2, true), SimpleStepSelectorBuilder {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio I think it would be better if run the example with both Cell Managers

Copy link
Author

Choose a reason for hiding this comment

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

@leolara Thank you!
I'll fix it!

let (_, constants_table) = ctx.sub_circuit(
single_config.clone(),
config.clone(),
poseidon_constants_table,
lens.n_inputs + 1,
);
let (_, matrix_table) =
ctx.sub_circuit(single_config, poseidon_matrix_table, lens.n_inputs + 1);
ctx.sub_circuit(config.clone(), poseidon_matrix_table, lens.n_inputs + 1);

let params = CircuitParams {
lens,
constants_table,
matrix_table,
};
let maxwidth_config = config(
MaxWidthCellManager::new(2, true),
SimpleStepSelectorBuilder {},
);
let (poseidon, _) = ctx.sub_circuit(maxwidth_config, poseidon_circuit, params);

let (poseidon, _) = ctx.sub_circuit(config, poseidon_circuit, params);

ctx.mapping(move |ctx, values| {
ctx.map(&poseidon, values);
Expand Down
172 changes: 164 additions & 8 deletions src/plonkish/compiler/cell_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,16 @@ impl CellManager for SingleRowCellManager {

#[derive(Debug, Default, Clone)]
pub struct MaxWidthCellManager {
max_width: usize,
max_width_advice: usize,
max_width_fixed: usize,
same_height: bool,
}

impl MaxWidthCellManager {
pub fn new(max_width: usize, same_height: bool) -> Self {
pub fn new(max_width_advice: usize, max_width_fixed: usize, same_height: bool) -> Self {
Self {
max_width,
max_width_advice,
max_width_fixed,
same_height,
}
}
Expand All @@ -264,7 +266,7 @@ impl CellManager for MaxWidthCellManager {
let mut forward_signal_row: usize = 0;

for forward_signal in unit.forward_signals.iter() {
let column = if placement.columns.len() <= forward_signal_column {
let column = if placement.forward.len() <= forward_signal_column {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio you are comparing signals with columns here, the left side should be forward_columns.len()

Copy link
Collaborator

Choose a reason for hiding this comment

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

advice_columns.len()

let column = if let Some(annotation) = unit.annotations.get(&forward_signal.uuid())
{
Column::advice(format!("mwcm forward signal {}", annotation), 0)
Expand All @@ -287,7 +289,7 @@ impl CellManager for MaxWidthCellManager {
);

forward_signal_column += 1;
if forward_signal_column >= self.max_width {
if forward_signal_column >= self.max_width_advice {
forward_signal_column = 0;
forward_signal_row += 1;
}
Expand All @@ -299,6 +301,59 @@ impl CellManager for MaxWidthCellManager {
forward_signal_row
} as u32;



let mut fixed_signal_column: usize = 0;
let mut fixed_signal_row: usize = 0;
let mut column_pos = self.max_width_advice + fixed_signal_column;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio we might not have used all the max_width_advice columns, for example max_width_advice can be 3, but there be only 2 forwards.

Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps column pos is equivalent to the (number of actual advice columns inserted (placement.columns.len() before adding fixed columns)) + fixed_signal_column; so this variable is not needed.

I think the cleaner solution is:

Be adding for advice and fixed, the columns to different arrays, and after all have been added then merge both arrays into placement.columns.

Copy link
Author

Choose a reason for hiding this comment

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

@leolara Thanks for your great advice and ideas !



for fixed_signal in unit.fixed_signals.iter() {
let column = if placement.fixed.len() <= fixed_signal_column {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio the left side should be fixed_columns.len()

let column = if let Some(annotation) = unit.annotations.get(&fixed_signal.uuid())
{
Column::fixed(format!("mwcm fixed signal {}", annotation))
} else {
Column::fixed("mwcm fixed signal")
};

placement.columns.push(column.clone());
column
} else {
column_pos += 1;
placement.columns[column_pos - 1].clone()
};

placement.fixed.insert(
*fixed_signal,
SignalPlacement {
column,
rotation: fixed_signal_row as i32,
},
);

fixed_signal_column += 1;
if fixed_signal_column >= self.max_width_fixed {
fixed_signal_column = 0;
fixed_signal_row += 1;
}
}

placement.base_height = if fixed_signal_row > forward_signal_row {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio you can assign to a new variable max of fixed_signal_row, forward_signal_row and use the result in one expression

Copy link
Author

Choose a reason for hiding this comment

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

@leolara Thank you for your comment!
Is it possible to represent it concisely with the following code?

let mut max_row = std::cmp::max(fixed_signal_row, forward_signal_row);
placement.base_height = if (fixed_signal_column != 0 && forward_signal_column != 0) {
    max_row as u32 + 1
} else {
    max_row as u32
};
placement.base_height = max_row;

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that code, you are setting max_row as mut, but you are not mutating it, also you are setting placement.base_height twice. Also, why are you checking both things for zero? If both are zero, max_row will be zero.

Copy link
Author

Choose a reason for hiding this comment

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

@leolara Since we don't know whether fixed_signal_row or forward_signal_row will be assigned to max_row, we need to check both xx_signal_column, don't we?
The xx_signal_column could be 0 even if the xx_signal_row is not 0.

if fixed_signal_column != 0 {
fixed_signal_row as u32 + 1
} else {
fixed_signal_row as u32
}
} else {
if forward_signal_column != 0 {
forward_signal_row as u32 + 1
} else {
forward_signal_row as u32
}
};


for step in unit.step_types.values() {
let mut step_placement = StepPlacement {
height: if forward_signal_column > 0 {
Expand Down Expand Up @@ -337,7 +392,7 @@ impl CellManager for MaxWidthCellManager {
step_placement.height = (internal_signal_row + 1) as u32;

internal_signal_column += 1;
if internal_signal_column >= self.max_width {
if internal_signal_column >= self.max_width_advice {
internal_signal_column = 0;
internal_signal_row += 1;
}
Expand Down Expand Up @@ -372,6 +427,7 @@ mod tests {
use crate::{
ast::{ForwardSignal, StepType},
plonkish::compiler::CompilationUnit,
plonkish::compiler::cell_manager::FixedSignal,
};

use super::{CellManager, MaxWidthCellManager};
Expand Down Expand Up @@ -402,7 +458,8 @@ mod tests {
// forward signals: a, b; step1 internal: c1, d, e; step2 internal c2

let cm = MaxWidthCellManager {
max_width: 2,
max_width_advice: 2,
max_width_fixed: 2,
same_height: false,
};

Expand Down Expand Up @@ -472,7 +529,8 @@ mod tests {
// forward signals: a, b; step1 internal: c1, d, e; step2 internal c2

let cm = MaxWidthCellManager {
max_width: 2,
max_width_advice: 2,
max_width_fixed: 2,
same_height: true,
};

Expand Down Expand Up @@ -515,4 +573,102 @@ mod tests {
1
);
}

#[test]
fn test_max_width_cm_different_columns() {
let mut unit = CompilationUnit::<()> {
forward_signals: vec![
ForwardSignal::new_with_phase(0, "forward signal 1".to_string()),
ForwardSignal::new_with_phase(0, "forward signal 2".to_string()),
],
fixed_signals: vec![
FixedSignal::new_with_id(0, "fixed signal 1".to_string()),
FixedSignal::new_with_id(0, "fixed signal 2".to_string()),
FixedSignal::new_with_id(0, "fixed signal 3".to_string()),
],
..Default::default()
};

let mut step1 = StepType::new(1500, "step1".to_string());
step1.add_signal("c1");
step1.add_signal("d");
step1.add_signal("e");
let mut step2 = StepType::new(1501, "step2".to_string());
step2.add_signal("c2");

let step1 = Rc::new(step1);
let step2 = Rc::new(step2);

unit.step_types.insert(1500, Rc::clone(&step1));
unit.step_types.insert(1501, Rc::clone(&step2));

// forward signals: a, b; step1 internal: c1, d, e; step2 internal c2

let cm = MaxWidthCellManager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio you should this test with different combinations of max_width_advice and max_width_fixed.

Including corner cases, like 0 and 1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If there are some repetation in the code use helper functions in the test module

max_width_advice: 2,
max_width_fixed: 2,
same_height: true,
};

cm.place(&mut unit);

assert_eq!(unit.placement.forward.len(), 2);
assert_eq!(unit.placement.fixed.len(), 3);

// placement.columns contains both advice and fixed columns
assert_eq!(unit.placement.columns.len(), 4);
assert_eq!(unit.placement.base_height, 1);

assert_eq!(
unit.placement
.steps
.get(&step1.uuid())
.expect("should be there")
.height,
3
);
assert_eq!(
unit.placement
.steps
.get(&step1.uuid())
.expect("should be there")
.signals
.len(),
3
);
assert_eq!(
unit.placement
.steps
.get(&step2.uuid())
.expect("should be there")
.height,
3
);
assert_eq!(
unit.placement
.steps
.get(&step2.uuid())
.expect("should be there")
.signals
.len(),
1
);
assert_eq!(
unit.placement
.fixed
.get(&unit.fixed_signals[0])
.expect("should be there")
.column,
unit.placement.columns[2]
);
assert_eq!(
unit.placement
.fixed
.get(&unit.fixed_signals[1])
.expect("should be there")
.column,
unit.placement.columns[3]
);

}
}
6 changes: 3 additions & 3 deletions tutorials/tutorial_pt3_ch2.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,9 +410,9 @@
],
"metadata": {
"kernelspec": {
"display_name": "chiquito_kernel",
"display_name": "Python 3",
"language": "python",
"name": "chiquito_kernel"
"name": "python3"
},
"language_info": {
"codemirror_mode": {
Expand All @@ -424,7 +424,7 @@
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.11.4"
"version": "3.10.0"
}
},
"nbformat": 4,
Expand Down