Fix Issue #469, Mapper error #478

nonhermitian · 2018-05-16T14:38:54Z

Description

Picks a better initial layout for register to physical qubits if none is given.

This is a stopgap fix, as the values should be cached as they need only be computed once.

Motivation and Context

Multiple issues with swap mapper, as reported in #469.

How Has This Been Tested?

Seems to solve all mapper issues.

Screenshots (if appropriate):

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.

nonhermitian · 2018-05-16T15:01:17Z

Waiting for @rraymondhp to confirm that his issues have also been resolved by this PR.

diego-plan9 · 2018-05-16T15:50:50Z

qiskit/_compiler.py

@@ -1,5 +1,7 @@
 # -*- coding: utf-8 -*-
 # pylint: disable=redefined-builtin
+# pylint: disable=C0103
+# pylint: disable=W9006


Can you remove this pylint exceptions and take care of the warning themselves instead? In general, disabling a linter check (specially in qiskit/*) should only be used as a last resort, not as a convenience, in order to maintain the consistency of the codebase.

Fixed the raising exception issue. Not going to worry about how my iterator constants are defined.

diego-plan9 · 2018-05-16T15:58:20Z

qiskit/_compiler.py

@@ -279,6 +294,54 @@ def load_unroll_qasm_file(filename, basis_gates='u1,u2,u3,cx,id'):
    return circuit_unrolled


+def best_subset(backend, n_qubits):


Would it be possible to have a couple of tests that exercise this function (or that invoke it trough compile())? Probably using the values that were reported in the initial issue or some edge cases would be a good idea.

I am trying to find a short test that raises an error.

atilag

atilag · 2018-05-16T15:59:43Z

qiskit/_compiler.py

+    cmap = np.asarray(backend.configuration['coupling_map'])
+
+    if n_qubits > device_qubits:
+        raise ValueError('Number of qubits greater than device.')


For exceptions raised because of a Qiskit logical error, we better use: QISKitError('text')

atilag · 2018-05-16T15:59:48Z

qiskit/_compiler.py

+    if n_qubits > device_qubits:
+        raise ValueError('Number of qubits greater than device.')
+    elif n_qubits <= 0:
+        raise ValueError('Number of qubits <= 0.')


Same as above: ValueError() => QISKitError()

atilag · 2018-05-16T16:04:06Z

qiskit/_compiler.py

+                connectivity mapping.
+    """
+    device_qubits = backend.configuration['n_qubits']
+    cmap = np.asarray(backend.configuration['coupling_map'])


Can we move this line after all the initial checks? ... so if some of the checks fail, there's no need for this computation.

atilag · 2018-05-16T16:06:30Z

qiskit/_compiler.py

+        raise ValueError('Number of qubits greater than device.')
+    elif n_qubits <= 0:
+        raise ValueError('Number of qubits <= 0.')
+    if n_qubits == 1:


I would put this check as the first line of the method. So if it's true we are returning fast.

atilag · 2018-05-16T16:19:39Z

qiskit/_compiler.py

+    best = 0
+    best_map = None
+    # do bfs with each node as starting point
+    for kk in range(sp_cmap.shape[0]):


Index variables are the only exception for one letter naming, doesn't hurt the readability though, but I'd encourage to rename it to just: k

atilag · 2018-05-16T16:29:43Z

qiskit/_compiler.py

+        bfs = cs.breadth_first_order(sp_cmap, i_start=kk, directed=False,
+                                     return_predecessors=False)
+
+        con = 0


This is some kind of match counter, right? Can we change the name to something more verbose/explanatory?

atilag · 2018-05-16T16:29:48Z

qiskit/_compiler.py

+
+        con = 0
+        for ii in range(n_qubits):
+            ss = bfs[ii]


Can we have a better naming for ss? bfs_node? (to distinguish from the node of the sparse matrix index)

atilag · 2018-05-16T16:31:28Z

qiskit/_compiler.py

+        ValueError: Wrong number of qubits given.
+    """
+    device_qubits = backend.configuration['n_qubits']
+    cmap = np.asarray(backend.configuration['coupling_map'])


Can we move this line before all the checks? so if something is failing in the preconditions there's no need to compute this.

diego-plan9

Fixed the raising exception issue. Not going to worry about how my iterator constants are defined.

Please refer to the contributor document and the code of conduct of the project - it is not about a single contributor having to worry, it is about maintaining the consistency and adhering to the conventions, in order to help the team as a whole. Again - can you remove the:

pylint: disable = C010

line, and take care of the linter issues in the proper way? They should be easier to fix after the latest round of renaming the variables to human-readable names, which is also in line with the rationale behind the linter and code style conventions.

* Fix mapper issue

nonhermitian added 2 commits May 16, 2018 10:35

Fix mapper issue

bfecd7b

fix lint issues

8eda5a2

diego-plan9 reviewed May 16, 2018

View reviewed changes

diego-plan9 changed the title ~~BUG: Fix Issue #469, Mapper error~~ Fix Issue #469, Mapper error May 16, 2018

nonhermitian added 2 commits May 16, 2018 12:15

fix linter

bce2a54

fix indent

505517f

atilag reviewed May 16, 2018

View reviewed changes

nonhermitian added 3 commits May 16, 2018 12:45

add test case

f5d9c08

style fixes

fe2c803

update test

0dac3b5

diego-plan9 suggested changes May 16, 2018

View reviewed changes

nonhermitian added 2 commits May 16, 2018 13:22

simplify test case

defddb3

Fixed counter names not cool enough for pylint

6be382d

atilag approved these changes May 16, 2018

View reviewed changes

too many blank lines

3ee07c1

diego-plan9 approved these changes May 16, 2018

View reviewed changes

atilag merged commit 64896d0 into Qiskit:master May 16, 2018

ajavadia mentioned this pull request May 30, 2018

Preserve i->i initial layout when coupling_map satisfied #527

Merged

9 tasks

lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this pull request Jul 30, 2019

Fix Issue Qiskit#469, Mapper error (Qiskit#478)

7e3a731

* Fix mapper issue

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Issue #469, Mapper error #478

Fix Issue #469, Mapper error #478

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!

Uh oh!

Uh oh!

Uh oh!

		@@ -279,6 +294,54 @@ def load_unroll_qasm_file(filename, basis_gates='u1,u2,u3,cx,id'):
		return circuit_unrolled


		def best_subset(backend, n_qubits):

Fix Issue #469, Mapper error #478

Fix Issue #469, Mapper error #478

Uh oh!

Conversation

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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!

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!