-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add support for physical qubits #2
Conversation
EDIT: Letting
Perhaps this requirement should be added somewhere. Presumably not in this repo, because |
It's transitive and already fixed - not our responsibility, and we'd just be polluting our repo if we did it. |
I provided a |
Running
Looking in These errors don't seem to be reported in CI. I haven't yet found how to use 3.10 to lint without modifying EDIT: setting EDIT: The following does what I want without modifying
|
That's pretty weird - looks like a bug in the pylint 2.14 series, because Fwiw, all the non-generated |
This is ready for review. In particular, the section "Notes" in the opening comment above describes the implementation.
|
d6d3093
to
3fe2d3b
Compare
3fe2d3b
to
5643ee8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this. I think this is a solid start. I think we'd be better leveraging the type system and pushing a lot of this work onto the type checker, rather than doing it ad-hoc throughout the higher-level components, but that's not too difficult a change. I also had a couple of data-representation points, but nothing super serious.
src/qiskit_qasm3_import/data.py
Outdated
class AddressingMode: | ||
"""Addressing mode for qubits in OpenQASM 3 programs. | ||
|
||
This class is useful as long as we allow only physical or virtual addressing modes, but | ||
not mixed. If the latter is supported in the future, this class will be modified or removed. | ||
""" | ||
|
||
# 0 == UNKNOWN | ||
# 1 == PHYSICAL | ||
# 2 == VIRTUAL | ||
|
||
def __init__(self): | ||
self._state = 0 | ||
|
||
def set_physical_mode(self): | ||
"""Set the addressing mode to physical. On success return `True`, otherwise `False`.""" | ||
if self._state != 2: | ||
self._state = 1 | ||
return True | ||
return False | ||
|
||
def set_virtual_mode(self): | ||
"""Set the addressing mode to virtual. On success return `True`, otherwise `False`.""" | ||
if self._state != 1: | ||
self._state = 2 | ||
return True | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a flat enum here with no interior mutability would be better. The interface added here still requires consumers to do the error checking, so I'm not sure it adds anything significant, and it ends up with behaviour of the importer being defined inside its data, rather than in the importer - the capabilities of the importer are being defined by methods on a separate piece of data in a separate file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commits are squashed in this PR. But I did use Enum
earlier: jlapeyre@a4491f4 . I didn't like that the details of how we switch modes and what is allowed, and how this is implemented, are spread across a few files and sections of code. I wanted it in one place and behind an abstraction. I noted in the commit message for the current code, jlapeyre@0834755, that the abstraction leaks. But if you compare the code you may agree that it is better this way than with Enum
. I considered raising the error in AddressingMode
, which would hide the abstraction at the cost of another call in the call stack. I think that would be a good trade.
I agree with your argument that this does not belong in data.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think that Enum
code looks much cleaner, and because it doesn't try to abstract very simple mutation away, it doesn't leak either. If you really wanted encapsulate the setter and erroring out, I'd use an enum and put the setter logic in a method on State
, but really I'd prefer to just revert exactly to that commit you had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take another try at a non-leaky abstraction. If it doesn't look good we can revert to Enum. My thought is that with a good abstraction you have one instantiation of an AddressingMode
(with no explicit initial data passed). And two calls in two files, one to set physical addressing, the other to set virtual. If there is a problem you get an error.
Compare to Enum
here: jlapeyre@a4491f4 . First, AddressingMode
should be, as you say, moved, to converter.py
. But it must be initialized with a value UNKNOWN
. Then in two different files there are two reads of the data and two mutations. If I have a good abstraction, then I change the implementation, or with luck even the semantics, without changing code at the call sites. But with Enum
I have to find and understand all five uses of the data and the logic surrounding them.
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
|
* Also test generated layout when in hw qubit addressing mode.
This needs to be fixed. For now, we include a test that requires the broken behavior.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we could just make the slightly safer typing in the SymbolTable
, we can merge this. 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. |
b170167
to
f0ac8e0
Compare
f0ac8e0
to
8b5da10
Compare
include 'stdgates.inc'; | ||
qubit p; | ||
""" | ||
with pytest.raises(ConversionError, match="already inserted in symbol table"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really a good error message. If these were really errors, we would trap it and throw a more specific error. Because trying to insert a symbol twice may arise from different operations. But since we really want to handle the messages as diagnostics, not errors, we need another way to get the same effect.
Use a stack instead of a recursive structure for managing nested symbol tables.
@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.
|
Fixes for a couple of outstanding issues:
|
This change increases performance by a factor of 1.5 or 2. Setting when already set takes about 65ns vs 120ns. Probably not a bottleneck at the moment.
Example
The most basic example is
which prints
Tasks
Additional Tasks
AddressingMode
resolve_condition
should not be optionalresolve_condition
should be of one type (State
)type.HardwareQubit
.ValueResolver.visit
no need to handle missing context explicitly.resolve
probably shouldn't acceptNone
as an input tocontext
here, since it's now required.resolve
or store in class, but don't mix. See Add support for physical qubits #2 (comment)ConvertVisitor.convert
fix inefficiency in iterating through keys.protected-access
lint exception more locally scoped.HardwareQubit
to the docs/index.rstAddressingMode
is being set correctly and propagated all the way up to the global scopeEnum
instances can (should) be compared by is checksSymbolTable
with stack-like structure.$q
.hardware_qubits
method. use a different design.Notes
EDIT: Regarding
_layout
andTranspileLayout
below: PR Qiskit/qiskit#9486 now provides an interface and some documentation.QuantumCircuit
has an undocumented (including in comments) property_layout
. It is initialized toNone
. It seems that sometimes it is set to aLayout
and sometimes aTranspileLayout
. I find that printing/drawing the circuit fails if_layout
is aLayout
, because the output methods reference_layout.initial_layout
, which does not exist. So I useTranspileLayout
, which does have_layout.initial_layout
.The hardware qubits are referred to by numbers. These are mapped
Qubit
s in aQuantumRegister
,"qr"
, in the order that they were seen in the OQ3 code; the first is mapped toqr[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 methodConvertVisitor.convert
.Here is the code that adds the 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 ofState
) because it contains the circuit, and each physical qubit must be added to the circuit as are the virtual qubits inConvertVisitor.visit_QubitDeclaration
.Closes #1
Closes the proxy issue Qiskit/qiskit#9435