Steve/jupyter notebook tutorial part 4 #15
Conversation
pychiquito/dsl.py
Outdated
| @@ -82,10 +83,21 @@ def pragma_disable_q_enable(self: Circuit) -> None: | |||
|
|
|||
| def add(self: Circuit, step_type: StepType, args: Any): | |||
| assert self.mode == CircuitMode.Trace | |||
| if self.num_step_instances >= self.ast.num_steps: | |||
| raise ValueError(f"Number of step instances exceeds {self.ast.num_steps}") | |||
| self.num_step_instances += 1 | |||
There was a problem hiding this comment.
@qwang98 isn' this the lenght of the self.witness.step_instances array? I don't think we need to keep self.num_step_instances.
There was a problem hiding this comment.
Deleted this field and use len(self.witness.step_instances) instead! Thanks. :)
pychiquito/dsl.py
Outdated
| @@ -22,6 +22,7 @@ def __init__(self: Circuit): | |||
| self.ast = ASTCircuit() | |||
| self.witness = TraceWitness() | |||
| self.rust_ast_id = 0 | |||
| self.num_step_instances = 0 | |||
There was a problem hiding this comment.
@qwang98 remember that all the witness generation fields have to be reset after a witness is generated because we can generate more than a witness for the circuit
There was a problem hiding this comment.
This isn't an issue, because TraceWitness() constructor creates an empty default witness before calling trace:
def gen_witness(self: Circuit, args: Any) -> TraceWitness:
self.mode = CircuitMode.Trace
self.witness = TraceWitness()
# ...
There was a problem hiding this comment.
Yes, but it does not set num_step_instances to 0.
|
The files conflict with the new files layout |
Fixed. |
|
@leolara Ready for review again. All tutorials first pass done. |
python/chiquito/dsl.py
Outdated
| @@ -23,6 +22,7 @@ def __init__(self: Circuit): | |||
| self.ast = ASTCircuit() | |||
| self.witness = TraceWitness() | |||
| self.rust_ast_id = 0 | |||
| self.num_step_instances = 0 | |||
There was a problem hiding this comment.
@qwang98 it seems we don't need this field anymore
There was a problem hiding this comment.
Surprises me why I didn't remove it already but fixed and thanks for catching. :)
|
@qwang98 this is approved |
Thanks! Will merge soon as I just finished the instance issue for core Chiquito. Need to update dependency here as well. |
WIP