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

Add ObservablesArray.apply_layout() #12221

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ihincks
Copy link
Contributor

@ihincks ihincks commented Apr 19, 2024

Summary

This PR adds the method ObservablesArray.apply_layout() which applies a transpiler layout to all elements of the array.

Details and comments

@ihincks ihincks added the mod: primitives Related to the Primitives module label Apr 19, 2024
@ihincks ihincks added this to the 1.1.0 milestone Apr 19, 2024
@ihincks ihincks requested review from a team as code owners April 19, 2024 03:58
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core
  • @ajavadia
  • @levbishop
  • @t-imamichi

@coveralls
Copy link

coveralls commented Apr 19, 2024

Pull Request Test Coverage Report for Build 9003200844

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 37 of 38 (97.37%) changed or added relevant lines in 1 file are covered.
  • 125 unchanged lines in 16 files lost coverage.
  • Overall coverage increased (+0.02%) to 89.645%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/primitives/containers/observables_array.py 37 38 97.37%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/routing/star_prerouting.py 1 97.75%
qiskit/circuit/library/data_preparation/initializer.py 1 93.75%
qiskit/synthesis/clifford/clifford_decompose_ag.py 2 96.15%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.02%
qiskit/circuit/library/standard_gates/xx_plus_yy.py 2 95.56%
qiskit/circuit/library/pauli_evolution.py 3 94.44%
qiskit/circuit/library/generalized_gates/linear_function.py 4 85.71%
crates/qasm2/src/lex.rs 5 92.62%
qiskit/circuit/library/generalized_gates/unitary.py 6 91.46%
qiskit/circuit/library/data_preparation/state_preparation.py 6 94.74%
Totals Coverage Status
Change from base Build 8930297650: 0.02%
Covered Lines: 62239
Relevant Lines: 69428

💛 - Coveralls

@t-imamichi
Copy link
Member

It looks good.
We need to elaborate #11594 so that we can minimize the overhead of this apply_layout.
Padding "I" for all observables could be too heavy. Nice to have a sparse representation ignoring "I".

@chriseclectic
Copy link
Member

chriseclectic commented Apr 24, 2024

This needs a bunch of unit tests, but other than that looks fine.

@t-imamichi agreed, though as long as we are happy with the function signature here we should add tests and merge, and then we can update its implementation to be more efficient later on when the internal data structure is improved for qubit sparsity.

@t-imamichi
Copy link
Member

@chriseclectic Yes, I agree with you. We can improve the performance as a separate PR.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I think the code for this function looks correct; although I'm not super familiar with ObservablesArray data structure. However, this is missing a release note (never mind this was never public before so there isn't a need to document this as an addition to the class's interface) and also has an unrelated test file change which is causing test job failures.

test/python/transpiler/test_transpile_layout.py Outdated Show resolved Hide resolved
This commit reverts accidental changes to the test module
test_transpile_layout. These changes were causing the test to fail
because the exepected layout was no longer complete.
@mtreinish mtreinish added the Changelog: None Do not include in changelog label May 3, 2024
ElePT
ElePT previously approved these changes May 3, 2024
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM

@ElePT ElePT enabled auto-merge May 3, 2024 11:00
ElePT
ElePT previously approved these changes May 3, 2024
@t-imamichi
Copy link
Member

t-imamichi commented May 3, 2024

I have two questions.

The current implementation uses the pauli in the last qubit if there are some duplicate indices.
If it is intentional, it is nice to add a test. Otherwise, it might be good to raise an error if there are some duplicate indices.

a = ObservablesArray({"XYZ": 1})
print(a.apply_layout([1, 1, 1]))
# ObservablesArray({'IXI': 1.0}, shape=())

I don't get the following behavior. Could you check whether it is correct or not? I thought 5 is out of bounds.

a = ObservablesArray({"XYZ": 1})
print(a.apply_layout([0, 1, 5]))
# ObservablesArray({'XYZ': 1.0}, shape=())

@ElePT ElePT disabled auto-merge May 3, 2024 13:29
@mtreinish
Copy link
Member

I don't think [1, 1, 1] is a valid layout array, that should be an error on input. [0, 1, 5] would be valid only if you set num_qubits >= 6 but without that it should error. My concern at this point then is tagging the release though, it sounds like you think this isn't ready to merge yet. If so then we can defer this until 1.2.0.

new_key = n_qubits * ["I"]
for char, qubit in zip(reversed(key), layout):
# Qubit position is from end of string
new_key[n_qubits - 1 - qubit] = char
Copy link
Member

Choose a reason for hiding this comment

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

if n_qubits - 1 - qubit < 0, the second case of my comment #12221 (comment) might occur.

It might be nice to enforce n_qubits - 1 - qubit is non-negative.

@mtreinish mtreinish modified the milestones: 1.1.0, 1.2.0 May 3, 2024
@mtreinish
Copy link
Member

mtreinish commented May 3, 2024

Given the issues flagged by @t-imamichi and that we're already past the release target date I think it's safer if we just defer this to 1.2.0 at this point instead of trying to rush this in at the last minute, so I've retargeted this PR for the next release.

@t-imamichi
Copy link
Member

I checked SparsePauliOp and found that the first case is same as SparsePauliOp and the second one raises an error.

In [11]: op = SparsePauliOp("XYZ")

In [12]: op
Out[12]:
SparsePauliOp(['XYZ'],
              coeffs=[1.+0.j])

In [13]: op.apply_layout([1, 1, 1])
Out[13]:
SparsePauliOp(['IXI'],
              coeffs=[0.-1.j])

In [14]: op.apply_layout([0, 1, 5])
---------------------------------------------------------------------------
QiskitError                               Traceback (most recent call last)
Cell In[14], line 1
----> 1 op.apply_layout([0, 1, 5])

File ~/tasks/4_2024/qiskit/terra/qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py:1163, in SparsePauliOp.apply_layout(self, layout, num_qubits)
   1161     n_qubits = num_qubits
   1162 if layout is not None and any(x >= n_qubits for x in layout):
-> 1163     raise QiskitError("Provided layout contains indices outside the number of qubits.")
   1164 if layout is None:
   1165     layout = list(range(self.num_qubits))

QiskitError: 'Provided layout contains indices outside the number of qubits.'

@chriseclectic
Copy link
Member

chriseclectic commented May 8, 2024

@t-imamichi For the current implementations of Pauli and SparsePauliOp, and this classes implementations, a list of ints for the layout is only able to apply a permutation of the current number of qubits unless you also specify num_qubits. This requirement is also stated in the apply_layout docstrings.

Eg these all work:

op1 = qi.Pauli("XYZ")
op2 = qi.SparsePauliOp(["XYZ"])
op3 = ObservablesArray(["XYZ"])
for op in [op1, op2, op3]:
    print(op.apply_layout([0, 1, 5], num_qubits=6))

If you remove the num_qubits kwarg, the qi ops error, while this current version does nothing, I've pushed a fix so that it will now raise the same errors as the qi ops if you don't do a permutation layout.

If we wanted to change this behavior to allow expansion via an integer layout for all apply_layout ops an easy fix would be to implicitly set num_qubits = 1 + max(layout) if num_qubits isn't provided and layout is a list of ints.

@t-imamichi
Copy link
Member

Thank you for the information, Chris. I think it's enough to make the behavior of ObservableArray.apply_layout compatible with that of SparsePauliOp.apply_layout.

@t-imamichi
Copy link
Member

t-imamichi commented May 9, 2024

(update) since #12385 was merged, the situation has changed. I will write a new comment below #12221 (comment). Please ignore this comment.


I'm checking the behavior of SparsePauliOp.apply_layout and ObservableArray.apply_layout.
I found some different cases as follows. I'm wondering how to make them compatible.

Case 1: negative indices

SparsePauliOp.apply_layout allows negative indices as follows while ObservableArray.apply_layout does not as follows.
Besides, the out-of-bound error for positive indices and negative indices are different. It raises QiskitError for positive indices and IndexError for negative indices. It would be update either one (IndexError might be more intuitive?)

op = SparsePauliOp("XYZ")
print(op.apply_layout([0, 1, -1])
# SparsePauliOp(['XYZ'],
#              coeffs=[1.+0.j])

print(op.apply_layout([0, 1, -2]))
# SparsePauliOp(['IXZ'],
#              coeffs=[0.-1.j])

print(op.apply_layout([0, 1, -3]))
# SparsePauliOp(['IYX'],
#              coeffs=[1.+0.j])

print(op.apply_layout([0, 1, -4]))
#    328 self._op_shape.compose(other._op_shape, qargs, front)
#    330 if qargs is not None:
#--> 331     x1, z1 = self.paulis.x[:, qargs], self.paulis.z[:, qargs]
#    332 else:
#    333     x1, z1 = self.paulis.x, self.paulis.z
#
#IndexError: index -4 is out of bounds for axis 1 with size 3

a = ObservablesArray({"XYZ": 1})
a.apply_layout([0, 1, -1])

#    170 new_key = n_qubits * ["I"]
#    171 for char, qubit in zip(reversed(key), layout):
#    172     # Qubit position is from end of string
#--> 173     new_key[n_qubits - 1 - qubit] = char
#    174 return "".join(new_key)
#
# IndexError: list assignment index out of range

Case 2: resulting coefficients for duplicate indices of layout

If we give duplicate indices, SparsePauliOp.apply_layout might return non-one coefficient while ObservableArray.apply_layout returns coefficient 1.
It does not make sense to me to change coefficients. Is it perhaps a bug of SparsePauliOp?

op = SparsePauliOp("XYZ")
print(op.apply_layout([0, 0, 0,])
# SparsePauliOp(['IIX'],
#               coeffs=[0.-1.j])

a = ObservablesArray({"XYZ": 1})
print(a.apply_layout([0, 0, 0])
# ObservablesArray({'IIX': 1.0}, shape=())

Case 3: layout whose size is smaller than the number of qubits

If we give a list whose size is smaller than the number of qubits as layout (e.g., []), SparsePauliOp.apply_layout and ObservableArray.apply_layout behave differently.

op = SparsePauliOp("XYZ")
print(op.apply_layout([]))
# File ~/tasks/4_2024/qiskit/terra/qiskit/quantum_info/operators/op_shape.py:477, in OpShape.compose(self, other, qargs, front)
#    475 ret._num_qargs_r = self._num_qargs_r
#    476 if len(qargs) != other._num_qargs_r:
#--> 477     raise QiskitError(
#    478         "Number of qargs does not match ({} != {})".format(
#    479             len(qargs), other._num_qargs_r
#    480         )
#    481     )
#    482 if self._dims_l or other._dims_l:
#    483     if self.dims_l(qargs) != other.dims_r():
#
# QiskitError: 'Number of qargs does not match (0 != 3)'

print(op.apply_layout([0], 1)
# File ~/tasks/4_2024/qiskit/terra/qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py:1157, in SparsePauliOp.apply_layout(self, layout, num_qubits)
#   1155 if num_qubits is not None:
#   1156     if num_qubits < n_qubits:
#-> 1157         raise QiskitError(
#   1158             f"The input num_qubits is too small, a {num_qubits} qubit layout cannot be "
#   1159             f"applied to a {n_qubits} qubit operator"
#   1160         )
#   1161     n_qubits = num_qubits
#   1162 if layout is not None and any(x >= n_qubits for x in layout):
#
# QiskitError: 'The input num_qubits is too small, a 1 qubit layout cannot be applied to a 3 qubit operator'

a = ObservablesArray({"XYZ": 1})
print(a.apply_layout([]))
# ObservablesArray({'': 1.0}, shape=())

print(a.apply_layout([0], 1))
# ObservablesArray({'Z': 1.0}, shape=())

@t-imamichi
Copy link
Member

I found a corner case of SparsePauliOp.apply_layout and made a PR #12375. ObservableArray.apply_layout works fine with the case. But it might be nice to port the tests.

@t-imamichi
Copy link
Member

I made a PR #12385 so that SparsePauliOp and Pauli raise an error for duplicate indices or negative indices.

@t-imamichi
Copy link
Member

t-imamichi commented May 10, 2024

There are some different behaviors between SparsePauliOp.apply_layout and ObservableArray.apply_layout as follows.

Case 1: negative indices

SparsePauliOp.apply_layout raises QiskitError with negative indices while ObservableArray.apply_layout raises IndexError. Since they both raise QiskitError with out-of-bounds indices. It's good to update ObservableArray.apply_layout to raise QiskitError with negative indices.

op = SparsePauliOp("XYZ")
print(op.apply_layout([0, 1, -1])
# QiskitError: 'Provided layout contains indices outside the number of qubits.'

a = ObservablesArray({"XYZ": 1})
a.apply_layout([0, 1, -1])

#    170 new_key = n_qubits * ["I"]
#    171 for char, qubit in zip(reversed(key), layout):
#    172     # Qubit position is from end of string
#--> 173     new_key[n_qubits - 1 - qubit] = char
#    174 return "".join(new_key)
#
# IndexError: list assignment index out of range

Case 2: duplicate indices

SparsePauliOp.apply_layout raises QiskitError with duplicate indices while ObservableArray.apply_layout allows it. I agree with Matthew that duplicate indices are not valid as a layout #12221 (comment).

op = SparsePauliOp("XYZ")
print(op.apply_layout([0, 0, 0])
# QiskitError: 'Provided layout contains duplicate indices.'

a = ObservablesArray({"XYZ": 1})
print(a.apply_layout([0, 0, 0])
# ObservablesArray({'IIX': 1.0}, shape=())

Case 3: layout whose size is smaller than the number of qubits

If we give a list whose size is smaller than the number of qubits as layout (e.g., []), SparsePauliOp.apply_layout and ObservableArray.apply_layout behave differently.
I'm wondering which behavior is more intuitive.

op = SparsePauliOp("XYZ")
print(op.apply_layout([]))
# File ~/tasks/4_2024/qiskit/terra/qiskit/quantum_info/operators/op_shape.py:477, in OpShape.compose(self, other, qargs, front)
#    475 ret._num_qargs_r = self._num_qargs_r
#    476 if len(qargs) != other._num_qargs_r:
#--> 477     raise QiskitError(
#    478         "Number of qargs does not match ({} != {})".format(
#    479             len(qargs), other._num_qargs_r
#    480         )
#    481     )
#    482 if self._dims_l or other._dims_l:
#    483     if self.dims_l(qargs) != other.dims_r():
#
# QiskitError: 'Number of qargs does not match (0 != 3)'

print(op.apply_layout([0], 1)
# File ~/tasks/4_2024/qiskit/terra/qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py:1157, in SparsePauliOp.apply_layout(self, layout, num_qubits)
#   1155 if num_qubits is not None:
#   1156     if num_qubits < n_qubits:
#-> 1157         raise QiskitError(
#   1158             f"The input num_qubits is too small, a {num_qubits} qubit layout cannot be "
#   1159             f"applied to a {n_qubits} qubit operator"
#   1160         )
#   1161     n_qubits = num_qubits
#   1162 if layout is not None and any(x >= n_qubits for x in layout):
#
# QiskitError: 'The input num_qubits is too small, a 1 qubit layout cannot be applied to a 3 qubit operator'

a = ObservablesArray({"XYZ": 1})
print(a.apply_layout([]))
# ObservablesArray({'': 1.0}, shape=())

print(a.apply_layout([0], 1))
# ObservablesArray({'Z': 1.0}, shape=())

@1ucian0
Copy link
Member

1ucian0 commented Jul 18, 2024

Is this still on-track for 1.2?

@ElePT
Copy link
Contributor

ElePT commented Jul 24, 2024

Pushing the milestone to 1.3, there are currently open questions related to the new SparseObservable implementation (#12671 ).

@ElePT ElePT modified the milestones: 1.2.0, 1.3.0 Jul 24, 2024
@ElePT
Copy link
Contributor

ElePT commented Oct 28, 2024

Hi @ihincks, is this PR still a priority for 1.3? I have noticed that there hasn't been a lot of activity recently to address @t-imamichi's concerns. To get into 1.3.0, it should be merged before the rc1 release date (November 7th).

@raynelfss raynelfss removed this from the 1.3.0 milestone Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: primitives Related to the Primitives module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants