Add support for physical qubits

[jlapeyre]

Example

The most basic example is

from qiskit_qasm3_import import parse

program = """
OPENQASM 3.0;

reset $2;
"""

circuit = parse(program)
circuit.draw()
print(circuit._layout)

which prints

0 ─|0>─       

TranspileLayout(initial_layout=Layout({
2: Qubit(QuantumRegister(1, 'qr'), 0)
}), input_qubit_mapping={Qubit(QuantumRegister(1, 'qr'), 0): 2})

Tasks

• [x] Refactor non-DRY parts
• [x] Tests
• [x] Raise some errors, for example when mixing types of qubits
• [x] Doc strings [No need for doc strings]
• [x] Release note

Additional Tasks

• [x] Rewrite AddressingMode
• [x] Split some tests into separate functions
• [x] Test (and implement) rejecting references to hardware qubits in function body.
• [x] Test hardware qubits in nested scopes.
• [x] [GJL] Test that layout is correctly generated for hardware qubits.
• [x] [GJL] Test hardware qubit first seen in local scope broken.
• [x] Remove commented out cruft in OQ code.
• [x] Second arg to resolve_condition should not be optional
• [x] Second arg to resolve_condition should be of one type (State)
• [x] Create and use a type for hardware qubits, eg type.HardwareQubit.
• [x] In ValueResolver.visit no need to handle missing context explicitly.
• [x] Check entry point resolve probably shouldn't accept None as an input to context here, since it's now required.
• [x] Either pass all state data in resolve or store in class, but don't mix. See https://github.com/Qiskit/qiskit-qasm3-import/pull/2#discussion_r1088943129
• [x] In ConvertVisitor.convert fix inefficiency in iterating through keys.
• [x] Make protected-access lint exception more locally scoped.
• [x] Disable too many lines lint check globally
• [x] AddressingMode: use enum for the mode number. Or switch to using just an enum
• [x] Add HardwareQubit to the docs/index.rst
• [x] Remove dead code, old comments, etc.
• [ ] test that the AddressingMode is being set correctly and propagated all the way up to the global scope
• [x] add a test that we can use hardware addressing in the main program, but still have user-defined gates
• [x] fairly sure Qiskit says that backends are required to label their hardware qubits starting from 0 with no gaps
• [ ] add an option to the top-level convert function to allow the user to specify the total size of the backend
• [x] Python Enum instances can (should) be compared by is checks
• [x] the triple-nested if/else now, can we refactor this a little to do early return
• [x] Rewrite SymbolTable with stack-like structure.
• [x] test that hardware qubits can be used outside the local scope they're first seen in
• [x] test that "hardware mode" being set in a local scope and then referencing virtual qubit after that scope is exited causes an error to be raised.
• [x] Remove test involving identifer $q.
• [x] Remove hardware_qubits method. use a different design.

Notes

EDIT: Regarding _layout and TranspileLayout below: PR https://github.com/Qiskit/qiskit-terra/pull/9486 now provides an interface and some documentation.

QuantumCircuit has an undocumented (including in comments) property _layout. It is initialized to None. It seems that sometimes it is set to a Layout and sometimes a TranspileLayout. I find that printing/drawing the circuit fails if _layout is a Layout, because the output methods reference _layout.initial_layout, which does not exist. So I use TranspileLayout, which does have _layout.initial_layout.

The hardware qubits are referred to by numbers. These are mapped Qubits in a QuantumRegister , "qr", in the order that they were seen in the OQ3 code; the first is mapped to qr[0] , and so on.

The references to hardware qubits are collected in ValueResolver.visit_Identifier. The layout is then created and attached to the circuit as the last step in the method ConvertVisitor.convert.

Here is the code that adds the layout

            names = filter(is_physical, symbols.keys())
            intlist = physical_qubit_identifiers_to_ints(names)
            qr = QuantumRegister(len(intlist), "qr")
            initial_layout = Layout.from_intlist(intlist, qr)
            input_qubit_mapping = dict(zip(qr, intlist))
            layout = TranspileLayout(initial_layout, input_qubit_mapping)
            state.circuit._layout = layout

Previously, the method ValueResolver.resolve takes only the node to be resolved and a symbol table. I added as a third argument the context (an instance of State) because it contains the circuit, and each physical qubit must be added to the circuit as are the virtual qubits in ConvertVisitor.visit_QubitDeclaration.

Closes #1

Closes the proxy issue https://github.com/Qiskit/qiskit-terra/issues/9435

[jlapeyre]

EDIT: Letting tox handle the environment is better. And already documented in this repo.

pylint -sn -rn src tests is failing in CI. This command failed to run locally under Python 3.11 because I had wrapt v1.12 installed in my virtual env. After upgrading to wrapt v1.14, the command runs correctly.

Perhaps this requirement should be added somewhere. Presumably not in this repo, because wrapt is an indirect dependency. If this has actually been fixed by a version requirement in the direct dependency that is using wrapt, we could make a requirement here. I have not tracked this down.

[jakelishman]

It's transitive and already fixed - not our responsibility, and we'd just be polluting our repo if we did it.

[jakelishman]

I provided a tox configuration, so you can run tox -e py311 to run the tests under Python 3.11, or tox -e lint to run the lint tests in an environment isolated from your current one, which should alleviate a lot of out-of-date environment issues.

[jlapeyre]

Running tox -e lint gives me the following

************* Module qiskit_qasm3_import.types
src/qiskit_qasm3_import/types.py:5:11: E1101: Module 'abc' has no 'ABC' member (no-member)
src/qiskit_qasm3_import/types.py:16:5: E1101: Module 'abc' has no 'abstractmethod' member (no-member)
src/qiskit_qasm3_import/types.py:58:4: W0231: __init__ method from base class 'Type' is not called (super-init-not-called)

Looking in .tox/lint/bin shows that this is using 3.11

These errors don't seem to be reported in CI. I haven't yet found how to use 3.10 to lint without modifying tox.ini.

EDIT: setting basepython = python3.10 in [testenv:lint] solves this problem, in the sense that the errors above are not reported.

EDIT: The following does what I want without modifying tox.ini:

tox -e lint -x testenv:lint.basepython=python3.10

[jakelishman]

That's pretty weird - looks like a bug in the pylint 2.14 series, because abc.abstractmethod and abc.ABC are defined as normal Python objects in the standard library. There isn't even any weird C-extension re-exports for those. I'll bump the pylint version to 2.15.x, since that has no errors with Python 3.11.

Fwiw, all the non-generated tox targets use the "system" python binary. It's not super ideal, but if you have a Python 3.10 environment that also has tox installed, you can do /path/to/python3.10 -m tox [tox options] (or /<py310 venv>/bin/tox [tox options]) to get it to do what you want here. (By "system", I just mean whatever's currently on the PATH as python, so virtual environments affect it.)

[jlapeyre]

This is ready for review.

In particular, the section "Notes" in the opening comment above describes the implementation.

This is nearly finished. You may want to take a look at this before I clean up rough edges it to see if we need structural changes to the implementation. In any case, I'll start polishing a bit.

[jlapeyre]

Because there were several items requested in the review, I added another checklist in the opening comment. All items except adding some more tests have been addressed.

Handling of the addressing mode is encapsulated in a class. This is slightly more complex structure-wise than using just an Enum jlapeyre@a4491f4, but not by much. It has the advantage that mutable details of the state of addressing mode are not spread across classes in two files. In general, it seems Qiskit prefers spreading mutation of state throughout the code. I suppose it is simpler in a sense and often is not problematic. In the present case in particular, reading and writing the Enum instead of using the present class is not really too complex; this code is not very big. I'm ok with using the Enum instead.

There are probably a few commented out print statements left to remove.

[jlapeyre]

Hardware qubits seen for the first time in local scope do not appear in the global symbol table. Local symbols are, correctly, discarded. In this case, the layout does not include these hardware qubits. I think this and other clumsy manipulations might be ameliorated by storing hardware qubits in a separate structure.

[jakelishman]

But that's because you're adding them to the local scope and not the global scope. They should be added to the global scope. That's not a clumsy manipulation, it's just following the rules of the language. Having multiple symbol tables is clumsy, and going to be a massive source of edge-case bugs if every lookup has to look in more than one place.

[jlapeyre]

I'm experimenting with implementing a class SymbolTable rather than using a simple dict. This does fix the one test for broken behavior. (i.e. the test for broken behavior now fails because the tested behavior is correct)

edit Following is fixed. However, other new bugs still exist.

[jlapeyre]

@jakelishman what does "per-run state" mean here? https://github.com/Qiskit/qiskit-qasm3-import/blob/246e999573c0d7a2162b779d9525b8b5b79c3cf0/src/qiskit_qasm3_import/expression.py#L56

I am updating doc strings. The doc string in question says that the instance state of ValueResolver is only symbols, which is no longer true. More concretely: e878653 (#2)

[jakelishman]

Oh, and to answer that question: I think I meant that ValueResolver doesn't have any mutable state that it generated/tracked as it traversed the expression tree, it just had a static lookup. That text isn't clear though, and I don't entirely remember what I was thinking.

[jlapeyre]

@jakelishman I changed the implementation of SymbolTable following your sketch above. There are a few bugs left (13 test failures). Before I finish though I wanted to think a bit more about design.

I think that moving logic for visibility into the SymbolTable (given that SymbolTable has a nice stack-like design for handling scopes) is the right thing for several reasons. But in raising exceptions we want to include information on the node; in particular the location in the source file. I want to just pass this information, for example the call would be symbol_table.get(symbol_name, node). This doesn't feel right because node and source information make the interface less clean. They are only for diagnostics. The same issue arose with AddressingMode and I chose the same solution; passing node.

Regarding the symbol table. We could do one of the following

1. Not pass node so that we lose the information for diagnostics. This results in a cleaner, less complex interface.
2. Pass node wherever it is needed for diagnostics. This means, even though symbol_table has a nice map-like interface to hide complexity, a simple lookup requires passing extraneous information (that will never be used for a correct input program)
3. Note the the context and node are almost always passed together. On recursive calls a different node is passed and the same context (possibly modified) is passed. Passing a light-weight structure, perhaps a named tuple, with these two pieces of information would hide some complexity. The arguments for this decreasing bugs are the standard ones, which I have already run into here: argument order, typos, etc. You have more complexity in one sense, but surface area for bugs is decreased. Same argument for why I like AddressingMode to be as struct.

At present, I think choosing 2. above is best. Get the kinks worked out, and then evaluate whether progressing to 3. is a good idea.

[jakelishman]

Yeah, I agree - it's not a perfect situation, but I don't think your number 2 is too tricky an interface, and still lets us emit diagnostics (at least the very limited set that we do).

[jlapeyre]

Yes, number two is the easiest way forward and doesnt sacrifice the user facing behavior that we want.

[jlapeyre]

@jakelishman I pushed more changes last night. Mainly this is using the new implementation of SymbolTable. Identifying errors in the test suite by checking strings was getting a bit complex. In the case of hardware qubits in a gate definiton, the error is raised in two places and would require keeping the strings in sync. I instead raise an error HardwareQubitInGateError.

EDIT I have yet to change the mapping of virtual to physical qubits. What I have now is in any case incorrect, because it ignores Qubit objects in the circuit and creates a new register. Further processing to take this to a backend would fail.

I think everything is finished.

Looking at how Layout works, I think we don't in fact need to worry about the number of hardware qubits or that the the must be consecutive integers starting with zero. If the OQ3 code refers to qubits $3 and $5 then we just need to specify in the layout a map for these to the Qubit objects in the QuantumCircuit. AFAICT unused hardware qubits are not referenced in the layout map. For example, the author of the OQ3 might know that a machine has 10 qubits and she wants to use numbers 3 and 5.

[jlapeyre]

I think we don't in fact need to worry about the number of hardware qubits ...

https://github.com/Qiskit/qiskit-terra/issues/9609

We do need to do something to better support the problem in the issue above.

[jlapeyre]

There's the question of "number of qubits in the backend", but let's do that in a follow-up.

A good idea. This PR discussion is already complicated.

[jlapeyre]

I started to add some tests for SymbolTable (7a412ea, 8e02af4) in order to make clear what we want it to do. This turned up some incorrect behavior.

[jlapeyre]

@jakelishman I redesigned the symbol table. It is a stack rather than a recursive structure. I think this is much more in line with what you were thinking about above. Furthermore the stack is managed by context managers.

EDIT:

Now the global symbol table is first on the stack. This is the natural way to implement the global symbols in this implementation. You still have to reference it in particular, knowing it holds the globals, in a couple of places.

There are still a couple of spots where the logic should be cleaned up. The biggest offenders have been fixed.

[jlapeyre]

Fixes for a couple of outstanding issues:

• I refactored until PhysicalQubitInGate was called in only one place. Then I replaced it with a call to raise_from_node. I check the identifier name (a str) rather than checking for types.HardwareQubit because I need to catch an illegal use where the hardware qubit's first appearance is in the gate, in which case it has not yet been constructed.

• I refactored _check_visible to remove redundant checks and also make the logic easier to read.