supporting non-string identifiers #308

1ucian0 · 2018-02-21T22:01:36Z

This PR allows using any kind of hashable (or None!) as a circuit name or register name.

Description

Any kind of hashable-type can be used as an identifier. Example: qp.create_quantum_register(2, 3) creates a 3-bit long quantum register with name 2, the integer.
Because the OpenQASM specification defines clearly the regex for an identifier (id := [a-z][A-Za-z0-9_]*, p22), no-string-typed identifiers are converted to this format when they are dump to QASM. This conversion is notified via logger (at info level) and is only performed once per identifier.
If you dont care about the names, you can just ignore that parameter. Example: qr = qp.create_quantum_register(size=3) creates a 3-bit long quantum register without any specific name. Internally, a string-typed name is created (which do not collide with existing names) and can be fetch with qr.name.
Going nameless allows QuantumProgram and Result to be more intuitive in some methods. For example, if you executed only one circuit, get_counts() will return the counts for that circuit without requiring the name.

Motivation and Context

One of the goals of QISKit is to provide a high-level interface to quantum programmers. Since it was so harnessed to OpenQASM it was natural to lift some of the QASM details to the user-space. As QISKit is moving out from this attachment (see, for example, #273), many of the low-level details are not necessary to expose any more. At least by default. Identifiers are one of those details.

Coders starting with QISKit see identifiers are unnecessarily or limiting (see #290). They are counter-intuitive for first timers. This PR allows, for example, to simplify the HelloWorld program from qiskit.org to:

from qiskit import QuantumProgram
bell = QuantumProgram()
qr = bell.create_quantum_register(size=2)
cr = bell.create_classical_register(size=2)
qc = bell.create_circuit(qregisters=[qr],cregisters=[cr])
qc.h(qr[0])
qc.cx(qr[0], qr[1])
qc.measure(qr[0], cr[0])
qc.measure(qr[1], cr[1])
result = bell.execute()
print(result.get_counts())

In addition, it pushed the OpenQASM identifier limitations to the point where it is relevant, i.e. when the high-level code needs to be converted to a QASM format. Identifiers can be almost anything now (fixes #290).

How Has This Been Tested?

This PR does not break backwards compatibility.
New tests were added to show and test the extra functionality.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

diego-plan9 · 2018-03-12T17:59:44Z

test/python/test_identifiers.py

+            }]
+        }
+        self.qp_program_finished = False
+        self.qp_program_exception = Exception()


Can you double-check if these are needed in all the test cases? They were meant to be for some specific async-related tests on test_quantumprogram.

Done in 91e78ae

diego-plan9 · 2018-03-12T18:06:03Z

qiskit/_register.py

        self.name = name
        self.size = size
+        self.oq_name = None


If self.oq_name depends only on self.name and it is always the same during the lifetime of the object, can we set its value during __init__ directly, instead of waiting until the openqasm_name() calls happen, avoiding one extra layer of complexity?

Additionally, can you rename the variable to something more descriptive (perhaps the full openqasm_name)?

because there is no need if the openqasm representation is never used (and we are trying to go in that direction).

I changed oq_name for _openqasm_name (to avoid collision the method) (in ef7e1fd).

openqasm_name() might be a property

done in e5a5fe4

diego-plan9 · 2018-03-12T18:08:37Z

qiskit/_register.py

+            self.oq_name = oq_name
+            return oq_name
+        self.oq_name = self.name
+        return str(self.name)


Related to the comment on __init__() - if that comment makes sense, we can probably simplify quite a bit the function, perhaps making it static (ie. def some_good_name(name)).

diego-plan9 · 2018-03-12T18:42:20Z

qiskit/_quantumcircuit.py

@@ -37,6 +37,7 @@ class QuantumCircuit(object):

    def __init__(self, *regs):
        """Create a new circuit."""
+        self.name = None


Does it make sense (conceptually) for self.name to hold the value None?

It seems to have the value None only for a brief moment, and to be always modified manually after its creation during create_circuit(). If that is the case, I'm wondering if we could actually consider name to be a required argument passed to the constructor directly: this way we would avoid ambiguity and lessen the risk of having the name in an inconsistent state. The major downside would be that we might break backwards compatibility (although in this case, it might not have such a big impact, as instantiating circuits is probably quite low-level).

Because the name is not given in QuantumCircuit construction time, but when the circuit is added to the quantum program. Yeap, I dont like that the circuits dont have a name per se neither. But I think in this way the original approach is kept (even when I'm not agreeing with it).

But if you are adding a name attribute to the class, you are effectively saying that "the circuits have a name" - and arguably modifying the original approach. Can you check the real impact of adding it to the constructor (as far as I can see, outside of the SDK there is only one tutorial that would need to be updated)? I feel that we should either modify the class completely, or not modify it at all, but standing in middle ground is not ideal (QuantumCircuit.name becomes a variable that is not really set by the class itself).

diego-plan9 · 2018-03-12T19:01:12Z

qiskit/_quantumprogram.py

+                name = qrms[0]
+            else:
+                raise QISKitError("You have to select a quantum register to get when there is more"
+                                  "than one available")


Nit: perhaps we can generalize this pattern in a small function name = get_name_if_there_is_only_one_item_or_raise_an_error(some_list) ? It seems to be used in a number of places - not sure if they are enough, though.

I went for get_single_name, it seems shorter :) (see a011417)

diego-plan9 · 2018-03-12T19:03:13Z

qiskit/_quantumprogram.py

@@ -387,14 +387,20 @@ def destroy_circuit(self, name):
            raise QISKitError("Can't destroy this circuit: Not present")
        del self.__quantum_program[name]

-    def add_circuit(self, name, quantum_circuit):
+    def add_circuit(self, name=None, quantum_circuit=None):


As discussed in private, the side effect of making an argument of the public API optional when it is not really optional is not ideal ... but still, I'm not sure about the best way of avoiding it. Tricky one!

diego-plan9 · 2018-03-12T19:19:05Z

qiskit/_quantumprogram.py

+        for i in range(10000):
+            if formatstring % i not in existing:
+                return formatstring % i
+        raise QISKitError("Anonymous identifier was not possible to be created")


Instead of the explicit loop, perhaps it makes sense to use a generator (or several) or similar, maybe with the help of itertools.count?

>>> some_counter = itertools.count() >>> next(some_counter) 0 >>> next(some_counter) 1 >>> next(some_counter) 2

range() does not requires imports, creates an object range and there is a limit to the looping.

Importing from the standard lib is "free", so it should not really weight too much in the decision - and actually the limit to the looping is not the best of practices, as it introduces some "arbitrary magic number" ™️ . If the standard library provides a pythonic way of solving a problem (in this case, generating sequential numbers), why not take advantage of it?

done in 41a20cd2948464d52c14b630de1c2aa28467bca7

I spent too much time trying this: qp.create_quantum_registers([{"quantum_registers": [ {"name": "qr", "size": 4} ] }]) unify format of the docstrings and general format fixes #290 create_program_with_specsnonames create_anonymous_classical_register create_anonymous_quantum_register create_classical_registers_noname create_quantum_registers_noname create_circuit_noname get_register_and_circuit_names_nonames get_qasm get_qasms reasonable defaults get_qasms_noname get_execution_list_noname change_circuit_qobj_after_compile_noname add_circuit_noname old QPS_SPECS gone convert the names to openqasm names when thats needed names can be ints tuples as id are also supported lint and style debugging appveyor Hash randomization causes the iteration order of dicts and sets to be unpredictable and differ across Python runs typ0!!! I forgot how to math checks about 0 and emtpy strings as identifiers better using id instead of hash lenght of the output considers short ids cleaning up self.oq_name -> self._openqasm_name get_single_name quantumcircuit name is set at new-time, not after test adjusted name arg counter to track automatic identifiers docstring and format openqasm_name as a property assertEqual -> assertCountEqual

* Tweak docstrings and exception messages when using non-string identifiers, for clarity towards the end user and for correctly indicating that the functions can use hashables. * Revise `get_single_name()`, renaming it and documenting in a more general way and making it privat-ish. * Revise `create_id()`, using strings directly as parameter rather than format strings, to avoid a potential source of errors. * Explicitly check that `add_circuit()` receives a valid `quantum_circuit`, preserving the previous behaviour. Note that this implies that the attribute is not really optional (even if declared optional by the signature).

diego-plan9 · 2018-03-15T11:10:02Z

Thanks, @1ucian0 ! I added a commit with a few minor changes plus rebasing, and finally merging. In practice it means that now identifiers for different entities (circuits, registers, etc) can be omitted while building the QuantumProgram, which hopefully will be more friendly towards developers. Nice!

* Support for non-string identifiers I spent too much time trying this: qp.create_quantum_registers([{"quantum_registers": [ {"name": "qr", "size": 4} ] }]) unify format of the docstrings and general format fixes Qiskit#290 create_program_with_specsnonames create_anonymous_classical_register create_anonymous_quantum_register create_classical_registers_noname create_quantum_registers_noname create_circuit_noname get_register_and_circuit_names_nonames get_qasm get_qasms reasonable defaults get_qasms_noname get_execution_list_noname change_circuit_qobj_after_compile_noname add_circuit_noname old QPS_SPECS gone convert the names to openqasm names when thats needed names can be ints tuples as id are also supported lint and style debugging appveyor Hash randomization causes the iteration order of dicts and sets to be unpredictable and differ across Python runs typ0!!! I forgot how to math checks about 0 and emtpy strings as identifiers better using id instead of hash lenght of the output considers short ids cleaning up self.oq_name -> self._openqasm_name get_single_name quantumcircuit name is set at new-time, not after test adjusted name arg counter to track automatic identifiers docstring and format openqasm_name as a property assertEqual -> assertCountEqual * Fixes for support of non-string identifiers * Tweak docstrings and exception messages when using non-string identifiers, for clarity towards the end user and for correctly indicating that the functions can use hashables. * Revise `get_single_name()`, renaming it and documenting in a more general way and making it privat-ish. * Revise `create_id()`, using strings directly as parameter rather than format strings, to avoid a potential source of errors. * Explicitly check that `add_circuit()` receives a valid `quantum_circuit`, preserving the previous behaviour. Note that this implies that the attribute is not really optional (even if declared optional by the signature).

1ucian0 requested review from diego-plan9, atilag and jaygambetta February 21, 2018 22:01

1ucian0 mentioned this pull request Feb 21, 2018

DAGCircuit Register Name #290

Closed

1ucian0 requested review from awcross1 and removed request for jaygambetta February 22, 2018 17:04

1ucian0 mentioned this pull request Feb 25, 2018

the compilation output should not depend on the Python version #313

Merged

diego-plan9 reviewed Mar 12, 2018

View reviewed changes

1ucian0 and others added 2 commits March 14, 2018 18:33

diego-plan9 merged commit e74c056 into Qiskit:master Mar 15, 2018

ajavadia mentioned this pull request Apr 13, 2018

C++ Simulator Refactor #386

Merged

9 tasks

1ucian0 mentioned this pull request Apr 14, 2018

Backends expect string as names in the JSON qobj #395

Merged

ajavadia mentioned this pull request Apr 22, 2018

Enforce string tokens? #420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

supporting non-string identifiers #308

supporting non-string identifiers #308

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

supporting non-string identifiers #308

supporting non-string identifiers #308

Uh oh!

Conversation

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!