Skip to content

Fix depth calculation for external gates#198

Merged
TheGupta2012 merged 9 commits intoqBraid:mainfrom
antalszava:bugfix-depth-calculation-external-gates
May 30, 2025
Merged

Fix depth calculation for external gates#198
TheGupta2012 merged 9 commits intoqBraid:mainfrom
antalszava:bugfix-depth-calculation-external-gates

Conversation

@antalszava
Copy link
Copy Markdown
Contributor

Summary of changes

This PR fixes how the depth of a QASM module is calculated when calling the depth method.

With the changes in this PR, a QASM module stores external gates that have been defined for it. At the moment, the main way to define external gates on a module is to pass the external_gates keyword argument to the unroll method. Calling the same unroll method with no such arguments being passed flushes the external gates of the module.

Storing such external gates on the module level helps the depth calculation: any external gates of the module contribute once to the overall depth of the module, instead of the definition of the external gate being considered.

Closes #77.

@antalszava antalszava requested a review from TheGupta2012 as a code owner May 29, 2025 10:12

# Unroll using any external gates that have been recorded for this
# module
qasm_module.unroll(external_gates = self._external_gates)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note: the design of this solution has been greatly influenced by this internal call: even though unroll may be called externally with the external_gates kwarg defined, internally here it has always been called without. Imho this can be problematic, hence the current approach that is being proposed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess external_gates is the only kwarg affecting the depth calculation so it is okay to use that here.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@@ -760,12 +761,13 @@ def _update_qubit_depth_for_gate(
qubit_node.num_gates += 1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Say we have the following program-

OPENQASM 3.0;
include "stdgates.inc";
gate my_gate() q {
 h q;
 x q;
}
qubit[2] q;
my_gate q[0];

According to the above logic, we shall have num_gates for qubit 0 as 3 whereas its depth would be 1 -

In [17]: import pyqasm

In [18]: qasm_str = """
    ...: OPENQASM 3.0;
    ...: include "stdgates.inc";
    ...: gate my_gate() q {
    ...:  h q;
    ...:  x q;
    ...: }
    ...: qubit[2] q;
    ...: my_gate q[0];"""

In [19]: mod = pyqasm.loads(qasm_str)

In [20]: mod.unroll(external_gates = ['my_gate'])

In [21]: mod._qubit_depths
Out[21]: 
{('q',
  0): QubitDepthNode(reg_name='q', reg_index=0, depth=1, num_resets=0, num_measurements=0, num_gates=3, num_barriers=0),
 ('q',
  1): QubitDepthNode(reg_name='q', reg_index=1, depth=0, num_resets=0, num_measurements=0, num_gates=0, num_barriers=0)}

This is happening because we update the num_gates both inside the two basic gates h and x, and while exiting the handling of the external gate itself! This property is used in the printer but the regression isn't detected as we don't have a visualization test with external gates.

Ideally, the check for self._recording_depth should be moved over the whole gate and depth update logic

self._unroll_barriers: bool = unroll_barriers
self._curr_scope: int = 0
self._label_scope_level: dict[int, set] = {self._curr_scope: set()}
self._recording_depth = True
Copy link
Copy Markdown
Member

@TheGupta2012 TheGupta2012 May 29, 2025

Choose a reason for hiding this comment

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

Let's name this to self._recording_ext_gate_depth as recording_depth sounds very general.
Then, True will mean we are inside the external gate and False otherwise.
Essentially, we just flip your current logic

Copy link
Copy Markdown
Member

@TheGupta2012 TheGupta2012 left a comment

Choose a reason for hiding this comment

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

Hi @antalszava thanks for working on this!
I've given some suggestions but overall the code looks good, will be happy to merge once they are addressed

antalszava and others added 2 commits May 29, 2025 14:59
@antalszava
Copy link
Copy Markdown
Contributor Author

Hi @TheGupta2012, thank you! I have made updates as per the suggestions.

@antalszava antalszava requested a review from TheGupta2012 May 29, 2025 13:16
Copy link
Copy Markdown
Member

@TheGupta2012 TheGupta2012 left a comment

Choose a reason for hiding this comment

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

Changes lgtm, thanks @antalszava !

@TheGupta2012 TheGupta2012 merged commit 7076d46 into qBraid:main May 30, 2025
20 checks passed
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.

[BUG] Depth calculation for external gates

3 participants