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

Classical conditioning on individual classical bits now supported #6018

Merged
merged 35 commits into from
May 25, 2021

Conversation

TharrmashasthaPV
Copy link
Contributor

@TharrmashasthaPV TharrmashasthaPV commented Mar 13, 2021

Summary

Fixes #1160 .

Details and comments

Before this PR the classical conditioning of gates only allowed conditioning on classical registers. As a follow up of discussions in #1160 , this PR allows conditioning gates on single classical bits too. Various tests have been included.

Known Issues:

  • Condition value validation required. Currently only the non-negativity condition of the condition value is checked. So, passing on any float or Boolean (for creg) also get accepted as a valid condition value which is not quite right. Also, bit length of the condition value is also not checked i.e., passing a m bit integer as the condition value for condition on a creg of n classical bits (where m>n) does not raise error .
  • All the drawers break when trying to draw a circuit that contains gates with classical conditioning on a single cbit.
  • qc.qasm() breaks when circuit qc contains gates with classical conditioning on a single cbit.
  • qc.depth() also breaks. Refer to Classical conditioning on individual classical bits now supported #6018 (comment)
  • disassemble(qc_assembled) breaks where qc_assembled is an assembled Qobj.
  • qc.num_connected_components breaks.
  • circuit_to_instruction(qc) breaks.
  • _check_wires_list() and substitute_node_with_dag() methods in qiskit/dagcircuit/dagcircuit.py break.
  • _is_same_c_conf() method in ForwardMatch and BackwardMatch classes in qiskit/transpiler/passes/optimization/template_matching breaks.
  • run() method in ConsolidateBlocks class in qiskit/transpiler/passes/optimization breaks.

@TharrmashasthaPV TharrmashasthaPV marked this pull request as ready for review March 31, 2021 18:53
@TharrmashasthaPV TharrmashasthaPV requested a review from a team as a code owner March 31, 2021 18:53
@TharrmashasthaPV TharrmashasthaPV changed the title [WIP] Classical conditioning on individual classical bits now supported Classical conditioning on individual classical bits now supported Apr 7, 2021
@1ucian0 1ucian0 self-assigned this Apr 9, 2021
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Thanks @TharrmashasthaPV for taking this on! This looks great so far. A few small comments inline.

Can you open separate issues for validation of the condition values and for raising gracefully when attempting to export to QASM2?

qiskit/assembler/assemble_circuits.py Outdated Show resolved Hide resolved
qiskit/assembler/assemble_circuits.py Outdated Show resolved Hide resolved
qiskit/assembler/assemble_circuits.py Outdated Show resolved Hide resolved
qiskit/circuit/instruction.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
Comment on lines 590 to 592
except StopIteration as ex:
raise DAGCircuitError('Did not find creg containing '
'mapped clbit in conditional.') from ex
Copy link
Member

@kdk kdk Apr 23, 2021

Choose a reason for hiding this comment

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

A nice generalization that's enabled by this PR (and maybe something to consider for a future issue/contribution) is to allow conditionals that span a list of arbitrary circuit Clbits (even if they live in different Registers). That would allow us to remove this DAGCircuitError (in the case where the condition doesn't map cleanly to a different register) and instead map the condition to a list of Clbits.

Copy link
Contributor Author

@TharrmashasthaPV TharrmashasthaPV Apr 29, 2021

Choose a reason for hiding this comment

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

So we ( me and @1ucian0 ) have been thinking about the generalization: conditioning on a list of Clbits. I would certainly be happy to work on it.

@@ -351,12 +352,19 @@ def _bits_in_condition(self, cond):
"""Return a list of bits in the given condition.

Args:
cond (tuple or None): optional condition (ClassicalRegister, int)
cond (tuple or None): optional condition (ClassicalRegister, int) or (Clbit, int)
Copy link
Member

@1ucian0 1ucian0 Apr 30, 2021

Choose a reason for hiding this comment

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

Suggested change
cond (tuple or None): optional condition (ClassicalRegister, int) or (Clbit, int)
cond (tuple or None): optional condition (ClassicalRegister, int), (Clbit, int), or (Clbit, bool)

@@ -320,13 +320,14 @@ def _check_condition(self, name, condition):

Args:
name (string): used for error reporting
condition (tuple or None): a condition tuple (ClassicalRegister,int)
condition (tuple or None): a condition tuple (ClassicalRegister,int),
(Clbit,Bool) or (Clbit,int)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(Clbit,Bool) or (Clbit,int)
(Clbit, bool) or (Clbit, int)

@kdk kdk added the Changelog: New Feature Include in the "Added" section of the changelog label May 19, 2021
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @TharrmashasthaPV , this looks great! Is there a release note covering this in any of the PRs? If not, it'd be good to add one here.

@TharrmashasthaPV
Copy link
Contributor Author

Thanks for taking this on @TharrmashasthaPV , this looks great! Is there a release note covering this in any of the PRs? If not, it'd be good to add one here.

Thanks a lot for the review @kdk . I have not added a release note yet. I will be sure to add one.

Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

@kdk kdk added the automerge label May 24, 2021
@mergify mergify bot merged commit 9f28ad7 into Qiskit:main May 25, 2021
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Jul 20, 2021
In Qiskit#6018 initial support for adding classical conditions on a single bit
instead of a register was added. However this wasn't accounted for in
qpy because it didn't exist when qpy was first written. This commit adds
support to qpy without a file format change so it can be backported and
used with already released versions of qpy without needing a new format
version. This is accomplished by exploiting the strict register naming
in qiskit. A register can't have a name outside of regex
"[a-z][a-zA-Z0-9_]*" which we leverage in the case of single clbit
conditions the "register" name in the output QPY data is set to
str(clbit_index) which isn't a valid name. Then on the loading side we
check for a valid name, if it's outside the allowed regex we treat the
condition as a single bit and the name is a str(index). The tradeoff here
is that for a QPY file generated with 0.18.1 (assuming this is backported
and included in 0.18.1) this qpy file can not be loaded with qiskit
0.18.0. But this is fine as we only guarantee compatibility in one
direction (loading qpy files generated with older with qiskit with a
newer version of qiskit).

Fixes partially Qiskit#6475
mergify bot added a commit that referenced this pull request Jul 29, 2021
* Handle single bit conditions in QPY

In #6018 initial support for adding classical conditions on a single bit
instead of a register was added. However this wasn't accounted for in
qpy because it didn't exist when qpy was first written. This commit adds
support to qpy without a file format change so it can be backported and
used with already released versions of qpy without needing a new format
version. This is accomplished by exploiting the strict register naming
in qiskit. A register can't have a name outside of regex
"[a-z][a-zA-Z0-9_]*" which we leverage in the case of single clbit
conditions the "register" name in the output QPY data is set to
str(clbit_index) which isn't a valid name. Then on the loading side we
check for a valid name, if it's outside the allowed regex we treat the
condition as a single bit and the name is a str(index). The tradeoff here
is that for a QPY file generated with 0.18.1 (assuming this is backported
and included in 0.18.1) this qpy file can not be loaded with qiskit
0.18.0. But this is fine as we only guarantee compatibility in one
direction (loading qpy files generated with older with qiskit with a
newer version of qiskit).

Fixes partially #6475

* Add release note

* Prefix bit index with null character

This commit modifies the special string we use in the register name
field to be prefixed with a null character. This leaves open using the
string for other special cases and also returning a sane error if an
invalid QPY file was generated by something besides Qiskit.

* Update qiskit/circuit/qpy_serialization.py

Co-authored-by: Jake Lishman <jake@binhbar.com>

* Add note to release note about feature not being supported

* Add backwards compat test case too

Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot pushed a commit that referenced this pull request Jul 29, 2021
* Handle single bit conditions in QPY

In #6018 initial support for adding classical conditions on a single bit
instead of a register was added. However this wasn't accounted for in
qpy because it didn't exist when qpy was first written. This commit adds
support to qpy without a file format change so it can be backported and
used with already released versions of qpy without needing a new format
version. This is accomplished by exploiting the strict register naming
in qiskit. A register can't have a name outside of regex
"[a-z][a-zA-Z0-9_]*" which we leverage in the case of single clbit
conditions the "register" name in the output QPY data is set to
str(clbit_index) which isn't a valid name. Then on the loading side we
check for a valid name, if it's outside the allowed regex we treat the
condition as a single bit and the name is a str(index). The tradeoff here
is that for a QPY file generated with 0.18.1 (assuming this is backported
and included in 0.18.1) this qpy file can not be loaded with qiskit
0.18.0. But this is fine as we only guarantee compatibility in one
direction (loading qpy files generated with older with qiskit with a
newer version of qiskit).

Fixes partially #6475

* Add release note

* Prefix bit index with null character

This commit modifies the special string we use in the register name
field to be prefixed with a null character. This leaves open using the
string for other special cases and also returning a sane error if an
invalid QPY file was generated by something besides Qiskit.

* Update qiskit/circuit/qpy_serialization.py

Co-authored-by: Jake Lishman <jake@binhbar.com>

* Add note to release note about feature not being supported

* Add backwards compat test case too

Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit afb0c47)
mergify bot added a commit that referenced this pull request Jul 29, 2021
* Handle single bit conditions in QPY

In #6018 initial support for adding classical conditions on a single bit
instead of a register was added. However this wasn't accounted for in
qpy because it didn't exist when qpy was first written. This commit adds
support to qpy without a file format change so it can be backported and
used with already released versions of qpy without needing a new format
version. This is accomplished by exploiting the strict register naming
in qiskit. A register can't have a name outside of regex
"[a-z][a-zA-Z0-9_]*" which we leverage in the case of single clbit
conditions the "register" name in the output QPY data is set to
str(clbit_index) which isn't a valid name. Then on the loading side we
check for a valid name, if it's outside the allowed regex we treat the
condition as a single bit and the name is a str(index). The tradeoff here
is that for a QPY file generated with 0.18.1 (assuming this is backported
and included in 0.18.1) this qpy file can not be loaded with qiskit
0.18.0. But this is fine as we only guarantee compatibility in one
direction (loading qpy files generated with older with qiskit with a
newer version of qiskit).

Fixes partially #6475

* Add release note

* Prefix bit index with null character

This commit modifies the special string we use in the register name
field to be prefixed with a null character. This leaves open using the
string for other special cases and also returning a sane error if an
invalid QPY file was generated by something besides Qiskit.

* Update qiskit/circuit/qpy_serialization.py

Co-authored-by: Jake Lishman <jake@binhbar.com>

* Add note to release note about feature not being supported

* Add backwards compat test case too

Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit afb0c47)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@1ucian0
Copy link
Member

1ucian0 commented Sep 6, 2021

Hi @TharrmashasthaPV
How far is this of pending things doing? Do you mind PRing a single release note once that the feature is fully supported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Classical conditioning on single classical bits
4 participants