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.

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

ZKR-1371-Update-Abstract-Interpretation-Code #23

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

FatemehHeidari
Copy link
Collaborator

There are some minor changes related to removing some TODOs and deleting an unused line of code.

And also a question from second round of code review:

This is more of a question then a suggestion. In the below function, do we intend on counting RegionColumn::Selector? I'm asking because in that case used == false but we call continue which skips the counting.
pub fn analyze_unconstrained_cells(&mut self) -> Result {
let mut count = 0;
for region in self.layouter.regions.iter() {
let selectors = HashSet::from_iter(region.selectors().into_iter());

        for (reg_column, rotation) in region.columns.iter().cloned() {
            let mut used = false;

            match reg_column {
                RegionColumn::Selector(_) => continue,
                RegionColumn::Column(column) => {
                    for gate in self.cs.gates.iter() {
                        for poly in gate.polynomials() {
                            let advices = abstract_expr::extract_columns(poly);
                            let eval = abstract_expr::eval_abstract(poly, &selectors);

                            column.index(); // <<<<<<<<<<<<<<<<<<<<<

                            if eval != AbsResult::Zero && advices.contains(&(column, rotation))
                            {
                                used = true;
                            }
                        }
                    }
                }
            };

            if !used {
                count += 1;
                self.log.push(format!("unconstrained cell in \"{}\" region: {:?} (rotation: {:?}) -- very likely a bug.", region.name,  reg_column, rotation));
            }
        }
    }

@jgorzny jgorzny removed the request for review from mmjahanara July 31, 2023 11:53
@@ -83,7 +83,6 @@ impl<'a, F: Field> Layouter<F> for &'a mut AnalyticLayouter<F> {

self.eq_table.insert(left, right);
Ok(())
//todo!("handle instance columns") ticket created: https://quantstamp.atlassian.net/browse/ZKR-1238
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be a real todo; I will push a commit to check our internal ticket and re-add the todo.

@lazypanda10117 lazypanda10117 self-requested a review August 24, 2023 01:06
@jgorzny jgorzny merged commit 001adfd into main Aug 24, 2023
@jgorzny jgorzny deleted the ZKR-1371-Update-Abstract-Interpretation-Code branch August 24, 2023 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants