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

Cell manager context #17

Open
wants to merge 2 commits into
base: circuit-tools-wip
Choose a base branch
from
Open

Cell manager context #17

wants to merge 2 commits into from

Conversation

CeciliaZ030
Copy link
Owner

@CeciliaZ030 CeciliaZ030 requested a review from Brechtpd July 5, 2023 06:45
Copy link
Collaborator

@Brechtpd Brechtpd left a comment

Choose a reason for hiding this comment

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

Ah yeah I temporarily removed it from the MPT code because it's currently not used!

To test it out it'll have to be integrated with the macro's/constraint builder because otherwise the code it untested no?

height_limit: usize,

// branch ctxs
branch_ctxs: HashMap<String, CmContext<F, C>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it a hashmap and not a simple vector? Contexts will work like a stack no? Similarly to how we handle the condition in the constraint builder. So if you enter a context you just push a new context on the stack, if you exit a context you just pop it from the stack?

Copy link
Owner Author

@CeciliaZ030 CeciliaZ030 Jul 6, 2023

Choose a reason for hiding this comment

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

Brechtpd#9 If you scrolled down to one of my replies:

I thought of it and figured that a dynamic branching callstack is equivalent to a static upside-down tree:

   |_if_| |_else_|     |_if_|
  |_____if_______|    |_else_|
 |__________________________|

A simple callstack structure won't work cuz we need to remember what happened in the past. Thus we have to save the entire tree.

In the condition case remember we had to use region_id to hack around this problem, basically manually divide the branching callstack into states labeled with id:

   |_if_| |_else_|     |_if_|
  |___ match____|    |_match__|
 |______________________________|
 (      START     )(   ACCOUNT   ) ....

And one time i said ideally we can make store expressions work like CM ctx by changing the macros, but I failed 😭 and that could be something we can do for the next iteration.

@@ -274,16 +288,87 @@ impl<F: Field, C: CellType> CellManager<F, C> {
expr: cells[0].expr(),
cells,
});
width += 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

width is just columns.len()? Why the extra variable?

Copy link
Owner Author

Choose a reason for hiding this comment

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

probably because in previous version there was a width? I'll get rid of it :)

@CeciliaZ030
Copy link
Owner Author

Ah yeah I temporarily removed it from the MPT code because it's currently not used!

To test it out it'll have to be integrated with the macro's/constraint builder because otherwise the code it untested no?

Ohh I forgot to add the macros back, will do it to see if it work.

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.

2 participants