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

Reorder result bits from real devices #430

Merged
merged 10 commits into from
May 3, 2018

Conversation

ajavadia
Copy link
Member

@ajavadia ajavadia commented Apr 26, 2018

Fixes #2 (!!)
Fixes #5
Fixes a bunch of other complaints

A current software bug in the remote devices is preventing the format of obtained classical bits from matching what the user expects. This PR addresses that.

With the implementation of qobj across the full stack, this problem will be resolved in the backends themselves. So this is a temporary solution until qobj is in place in all backends.

Description

The reordering is implemented in the ibmqbackend and is easy to turn on/off.
Includes reordering, truncating to match the circuit clbits, and inserting spaces to signify separate classical registers.

Motivation and Context

Numerous bug reports, so I am submitting this even though qobj is close.

How Has This Been Tested?

Wrote 2 tests to check the reordered result against the simulator results, which are correct.

Screenshots (if appropriate):

Example circuit:

q0 = QuantumRegister(3)
q1 = QuantumRegister(2)
q2 = QuantumRegister(1)
c0 = ClassicalRegister(3)
c1 = ClassicalRegister(2)
c2 = ClassicalRegister(1)
qc = QuantumCircuit(q0, q1, q2, c0, c1, c2)
qc.h(q0[0])
qc.cx(q0[0], q0[2])
qc.x(q1[1])
qc.h(q2[0])
qc.barrier()
qc.measure(q0[0], c2[0])
qc.measure(q0[1], c0[1])
qc.measure(q0[2], c1[1])
qc.measure(q1[0], c0[0])
qc.measure(q1[1], c1[0])
qc.measure(q2[0], c0[0])

Output from ibmqx5, before:
image

Now:
image

Simulator:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

"""Tests for bit reordering fix."""

import unittest
import qiskit as qk
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you use the fully qualified name (ie. import qiskit), for consistency?

Copy link
Member

Choose a reason for hiding this comment

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

fixed in b658084



@unittest.skipIf(not real, 'no remote device available.')
class TestBitReordering(QiskitTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Can you revise these tests? They seem to be raising an Exception intermitently:

qiskit/wrapper/_wrapper.py:150: in execute
    wait, timeout, skip_translation)
qiskit/_compiler.py:77: in execute
    result = backend.run(q_job)
qiskit/backends/ibmq/ibmqbackend.py:125: in run
    _reorder_bits(this_result)  # TODO: remove this after Qobj

    def _reorder_bits(result):
        """temporary fix for ibmq backends.
        for every ran circuit, get reordering information from qobj
        and apply reordering on result"""
        for idx, circ in enumerate(result._qobj['circuits']):
    
            # device_qubit -> device_clbit (how it should have been)
            measure_dict = {op['qubits'][0]: op['clbits'][0]
                            for op in circ['compiled_circuit']['operations']
                            if op['name'] == 'measure'}
    
            res = result._result['result'][idx]
            counts_dict_new = {}
>           for item in res['data']['counts'].items():
E           TypeError: string indices must be integers

when running against ibmqx4 at least. It seems it might be related to a timeout which is not handled properly during run() - the contents of result._result on one of those failures where:

{'backend': 'ibmqx4',
 'job_id': '(redacted)',
 'name': '3ZwDQis6FwX0j5gRpYvmcAEZ273upm',
 'result': 'QISkit Time Out',
 'status': 'ERROR'}

From a practical point of view, they are also a bit problematic: since they depend on a real device, the running time of the tests can be rather large (in my tests, they ranged from 150 to 170 seconds, which is rather large considering that the current running time for all the test suite is ~120 seconds. I'd really like to avoid that one way or the other, by making them opt-in or similar - any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

fixed in 5e99aed

@ajavadia
Copy link
Member Author

ajavadia commented May 2, 2018

This should be ready to merge now. Thanks @diego-plan9 for the catch and @1ucian0 for the help.

Move the `_authenticate()` call to inside `TestBitReordering`, to avoid
`_requires_qe_access` raising an `SkipTest` during test discovery
(at module import time).
@diego-plan9
Copy link
Member

Thanks @ajavadia and @1ucian0 for fixing the long-standing bugs! 🎉

@diego-plan9 diego-plan9 merged commit 07b2485 into Qiskit:master May 3, 2018
@msegado
Copy link

msegado commented May 7, 2018

Thanks for adding this! I'm wondering - will this be released as a bugfix in 0.4.x or as a breaking change in 0.5.x? (Some of my code also does reordering to work around #2, so when this fix lands I'll need to stop doing that to avoid reordering twice!)

@ajavadia
Copy link
Member Author

ajavadia commented May 7, 2018

@msegado we will add this to the 0.5 release, which is close (about a week away).

@msegado
Copy link

msegado commented May 7, 2018

@ajavadia Got it, thanks for the info!

@ajavadia ajavadia deleted the reorder-result branch July 21, 2018 00:08
lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this pull request Jul 30, 2019
* _reorder_bits method in ibmqbackend for real backends

* add reordering tests

lint

* qk -> qiskit

* SKIP_SLOW_TESTS

* check the status is COMPLETED before reordering

* local_qiskit_simulator -> local_qasm_simulator

* style

* revise timeout for test_reordering

* Reorganize authentication in test_reordering

Move the `_authenticate()` call to inside `TestBitReordering`, to avoid
`_requires_qe_access` raising an `SkipTest` during test discovery
(at module import time).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Classical bit string in strange format for real device. Behavior of the instruction "measure q[i] -> q[j];"
6 participants