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

Conditionals must be updated to match schema #1778

Merged

Conversation

kdk
Copy link
Member

@kdk kdk commented Feb 8, 2019

Addresses #1283 by updating assemble_circuits to replace QASM-style conditionals with a bfunc+Qobj-conditional. Also, updates qobj json schema to allow bfuncs with single integer register and memory attributes.

@kdk
Copy link
Member Author

kdk commented Feb 8, 2019

Should be on hold until #861 supports qobj-conditionals for basic aer.

@diego-plan9
Copy link
Member

Also, updates qobj json schema to allow bfuncs with single integer register and memory attributes.

Can you add the type: specs label, if indeed the PR contains changes to the schema? It would help in collecting the list of schema changes and book-keeping in general.

@kdk kdk added the type: specs Issues and PRs that involve modifications to the open specs label Feb 8, 2019
@kdk
Copy link
Member Author

kdk commented Mar 11, 2019

Can be taken off on-hold after #1955 merges.

@ajavadia ajavadia removed the on hold Can not fix yet label Mar 11, 2019
@ajavadia
Copy link
Member

can you please rebase this? circuits_to_qobj is now deprecated in favor of qiskit.compiler.assembler (#1856) and there are a couple changes to qobj (#1909)

@ajavadia ajavadia added this to the 0.8 milestone Mar 19, 2019
@kdk kdk force-pushed the 1283-conditionals-must-be-updated-to-match-schema branch from 557f864 to 8d58111 Compare April 5, 2019 15:46
bfunc's were defined previously, but a higher precedence condition
required `register` and `memory` attributes to be arrays on all QASM
instructions, while bfuncs define them as single integers. This
fix moves the "memory/register should be an array" condition to the
specific QASM instructions which require it.
@kdk kdk force-pushed the 1283-conditionals-must-be-updated-to-match-schema branch from 8d58111 to 8207686 Compare April 5, 2019 15:47
test/python/compiler/test_assembler.py Outdated Show resolved Hide resolved
test/python/compiler/test_assembler.py Outdated Show resolved Hide resolved
test/python/compiler/test_assembler.py Outdated Show resolved Hide resolved
test/python/compiler/test_assembler.py Outdated Show resolved Hide resolved
@ajavadia
Copy link
Member

ajavadia commented Apr 7, 2019

The 'conditional' field in a qobj should be an integer right (conditioned on a single register bit)? I wonder why the schema has it as follows:

                "conditional": {
                    "oneOf": [
                        {
                            "minimum": 0,
                            "type": "integer"
                        },
                        {
                            "type": "object"
                        }
                    ]

                },

It should definitely NOT be an array. But the example given in #1283 seems to expect an array. @chriseclectic can you double check that in Aer you don't expect an array?

With this patch, that example correctly outputs integers for the conditionals:

[QasmQobjInstruction(mask='0x3', name='bfunc', register=2, relation='==', val='0x0'),
 QasmQobjInstruction(conditional=2, name='x', qubits=[0]),
 QasmQobjInstruction(mask='0x3', name='bfunc', register=3, relation='==', val='0x1'),
 QasmQobjInstruction(conditional=3, name='y', qubits=[0]),
 QasmQobjInstruction(mask='0x3', name='bfunc', register=4, relation='==', val='0x2'),
 QasmQobjInstruction(conditional=4, name='z', qubits=[0]),
 QasmQobjInstruction(mask='0x3', name='bfunc', register=5, relation='==', val='0x3'),
 QasmQobjInstruction(conditional=5, name='h', qubits=[0])]

@kdk kdk force-pushed the 1283-conditionals-must-be-updated-to-match-schema branch from 8207686 to 2b406e3 Compare April 8, 2019 19:55
@kdk
Copy link
Member Author

kdk commented Apr 8, 2019

@ajavadia Updated, thanks for the comments.

Agree that conditonals in the schema can be either integers (qobj-style) for objects (deprecated qasm-style). I don't know of a case where they would be an array.

aer checks for an integer, and falls back to assuming an object:
https://github.com/Qiskit/qiskit-aer/blob/e002af4f02cb956d8a9d73881d9e63e6a9e5c162/src/framework/operations.hpp#L511
basic-aer does the same:
https://github.com/Qiskit/qiskit-terra/blob/b123277669445bf1e8b1e2533c6db9d4a4ed0e84/qiskit/providers/basicaer/qasm_simulator.py#L496

@ajavadia ajavadia merged commit d5b2a39 into Qiskit:master Apr 10, 2019
lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this pull request Jul 30, 2019
* Update qobj JSON schema to allow bfuncs.

bfunc's were defined previously, but a higher precedence condition
required `register` and `memory` attributes to be arrays on all QASM
instructions, while bfuncs define them as single integers. This
fix moves the "memory/register should be an array" condition to the
specific QASM instructions which require it.

* Update circuits_to_qobj to emit qobj-compliant conditionals.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: specs Issues and PRs that involve modifications to the open specs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants