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

Circuit::num_gates not consistent with TKET n_gates #105

Closed
lmondada opened this issue Sep 14, 2023 · 4 comments · Fixed by #384
Closed

Circuit::num_gates not consistent with TKET n_gates #105

lmondada opened this issue Sep 14, 2023 · 4 comments · Fixed by #384
Assignees

Comments

@lmondada
Copy link
Contributor

The following surprised me. Consider a circuit made of a single rotation:

c = Circuit(1).Rz(0.5, 0)

Obviously, in TK1 we have

c.n_gates # this is 1

However, in TK2, we get

let c_str = r#"{"bits": [], "commands": [{"args": [["q", [0]]], "op": {"params": ["0.5"], "type": "Rz"}}], "created_qubits": [], "discarded_qubits": [], "implicit_permutation": [[["q", [0]], ["q", [0]]]], "phase": "0.0", "qubits": [["q", [0]]]}"#;
let ser: circuit_json::SerialCircuit = serde_json::from_str(c_str).unwrap();
let circ: Hugr = ser.decode().unwrap();
circ.num_gates() // this is 3

After reducing it to this example, it became clear to me that the overhead are the constant nodes introduced, but this is confusing.

@cqc-alec
Copy link
Collaborator

At least num_gates() seems like the wrong name; should be num_nodes() or something.

@lmondada
Copy link
Contributor Author

I would also be fine with the current behaviour, but then we should

  • make it explicit in the docs
  • have another method that counts gates TK1-style; I need that for benchmarking (suggestion: num_q_gates).

@aborgna-q
Copy link
Collaborator

We should define what is a gate somewhere (clearly it's not the same as an Op).

@ss2165
Copy link
Member

ss2165 commented Sep 20, 2023

I would suggest avoiding "gate" as an additional concept, and instead something like num_q_ops

@aborgna-q aborgna-q self-assigned this Mar 4, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 6, 2024
Closes #105. Closes #108.

`num_gates` used to count every node in the top-level region, giving
unexpected results on results with constants, control flow, or anything
other than simple gates.

`num_operations` now only counts `CustomOp`s, traversing containers as
needed.

I also improved the circuit unit tests, to include circuits in modules
and circuits in `FuncDefn`s (instead of `DFG`s).

Some notes:
- Part of the tests testing parametric operations is commented out until
we solve CQCL/hugr#1166.
- Although the test circuits have function names, `Circuit::name`
returns `None`. I'll address that in another PR.
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 a pull request may close this issue.

4 participants