Skip to content

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Dec 15, 2020

Follow me down this rabbit hole. I'll be your guide:

@kdk noticed that the test test.python.compiler.test_transpiler.TestTranspile.test_wrong_initial_layout was passing for the wrong reasons (problem 1). Here is the test:
https://github.com/Qiskit/qiskit-terra/blob/a46af7daa86b2d9c21f79dc5a1bfc1a09fcc3d4f/test/python/compiler/test_transpiler.py#L410-L427

Indeed, this tries to transpile on a badly defined initial_layout parameter. However, transpile(qc, backend, initial_layout=bad_initial_layout) raises with the following message:

qiskit.dagcircuit.exceptions.DAGCircuitError: 'duplicate register q'

This exception is raised by EnlargeWithAncilla because is trying to add QuantumRegister(3, 'q') (from initial_layout into the dag that already has QuantumRegister(2, 'q').

That happens because FullAncillaAllocation wrongly allowed QuantumRegister(3, 'q') to be part of the layout (problem 2). FullAncillaAllocation should check that the register mentioned in the layout also exist in the circuit.

When that validation was added, SabreLayout was raising because it was trying to use a layout with ancillas. The error occurred in this loop:
https://github.com/Qiskit/qiskit-terra/blob/54c8870623e28cb3f9f4eb92828fb8fd5f9dde49/qiskit/transpiler/passes/layout/sabre_layout.py#L97-L107

The PassManager in this code is changing the initial_layout variable in-place (Problem 3).

Here is a summary of this PR:

  • Problem 1: rewriting test_wrong_initial_layout to test what it suppose to test.
  • Problem 2: extend FullAncillaAllocation with inital_layout validation. And test it.
  • Problem 3: When SetLayout, copy the layout object to avoid changing initial_layout in-place.

@1ucian0 1ucian0 requested a review from a team as a code owner December 15, 2020 17:43
@kdk kdk added this to the 0.17 milestone Dec 18, 2020
@kdk kdk added the Changelog: API Change Include in the "Changed" section of the changelog label Dec 18, 2020
@mergify mergify bot merged commit 3947f25 into Qiskit:master Dec 22, 2020
molar-volume pushed a commit to molar-volume/qiskit-terra that referenced this pull request Dec 26, 2020
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
mergify bot added a commit that referenced this pull request Jan 13, 2021
* 1. params are required in convert method in GradientBased and HessianBased subclasses
2. fix on handling params of type ParameterExpression

* Update qiskit/opflow/gradients/gradient.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* passmanager(..., callback=...) parameter removed (#5522)

* passmanager callback removal

* unused-import

* reno

* better initial_layout validation (#5526)

Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>

* 1) unit tests modified to test ParameterVector input for Gradient, NaturalGradient, QFI and Hessian
2) index method added to ParameterVector
3) handling of ParameterVector changed for Hessian to be consistent with list params

* Hessian with respect to vector-like params returns matrix, unit test modified accordingly

* Add PiecewiseChebyshev arithmetic circuit (#5364)

* general polynomial approximation

* release notes

* fix qubit re-allocation

* fix re-calling build

* Update qiskit/circuit/library/arithmetic/piecewise_chebyshev.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update qiskit/circuit/library/arithmetic/piecewise_chebyshev.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update qiskit/circuit/library/arithmetic/piecewise_chebyshev.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* fix indentation lint error

* move tests to library module

* Add suggestions from code review

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Ensure aux_operator eigenvalues are normalized (#5496)

As reported on multiple occasions in Qiskit Aqua ([1], [2]), the
eigenvalues of the auxiliary operators are not correctly normalized when
the QASM backend is used.

This commit fixes this short-coming by applying the same normalization
to the VQE's eigenstate when obtained from a QASM backend (in which case
this is a dictionary) as done by the `CircuitSampler` in its
`sample_circuits` function.
The case of the statevector backend is unaffected by this change, as it
will return the eigenstate as a list.

A unittest is added which asserts this behavior.

[1]: https://github.com/Qiskit/qiskit-aqua/issues/1460
[2]: qiskit-community/qiskit-aqua#1467

Co-authored-by: Julien Gacon <gaconju@gmail.com>

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Hessian with respect to vector-like param returns matrix, unit test modified accordingly

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Luciano Bello <luciano.bello@ibm.com>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
Co-authored-by: Almudena Carrera Vazquez <almudenacarreravazquez@hotmail.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants