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.

Sign up for GitHub

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

Jump to bottom

marginal_distribution raises for NumPy array indices #8283

Closed
nonhermitian opened this issue Jul 1, 2022 · 2 comments · Fixed by #8288
Closed

marginal_distribution raises for NumPy array indices #8283

nonhermitian opened this issue Jul 1, 2022 · 2 comments · Fixed by #8288
Labels
bug Something isn't working good first issue Good for newcomers stable backport potential The bug might be minimal and/or import enough to be port to stable

Comments

@nonhermitian
Copy link
Contributor

Environment

  • Qiskit Terra version: 0.21
  • Python version: 3.10
  • Operating system: osx

What is happening?

marginal_distribution raises error when passes a NumPy array of indices:

marginal_distribution(counts, np.asarray([0, 2]))

--> 225 if indices is not None and (not indices or not set(indices).issubset(range(num_clbits))):
226 raise QiskitError(f"indices must be in range [0, {num_clbits - 1}].")
228 if isinstance(counts, Counts):

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

How can we reproduce the issue?

Try above

What should happen?

It should work for both lists and arrays.

Any suggestions?

No response
@nonhermitian nonhermitian added the bug Something isn't working label Jul 1, 2022
@Cryoris Cryoris added the good first issue Good for newcomers label Jul 1, 2022
@nonhermitian nonhermitian mentioned this issue Jul 1, 2022
Merged
@mtreinish
Copy link
Member

This should be easy to fix and backport for 0.21.1, we just need to change the is empty check on the input validation there to something that works with lists and numpy arrays. I'm pretty sure the python->rust boundary will work fine as is if it gets a numpy array (although it won't be as efficient as declaring an input type as a numpy array, which is easy to add support for in addition to a generic sequence input)

@nonhermitian
Copy link
Contributor Author

Yeah it is a simple fix. Was going to do it myself but saw that I would need to modify type hints, and I draw the line at type hinting.

@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jul 1, 2022
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Jul 1, 2022
@mtreinish
Fix handling of numpy arrays for indices in marginal_distribution
c22f5f9 
This commit fixes an issue with the marginal_distribution() function
where it would previously error if a numpy arrray of ints was passed in
for the indices parameter. This was caused by the input validation
looking for empty input by checking `not indices` which isn't valid for
numpy arrays. This commit fixes this by adjusting the check for
empty lists to work with any sequence input.

Fixes Qiskit#8283
@mtreinish mtreinish mentioned this issue Jul 1, 2022
Merged
@mergify mergify bot closed this as completed in #8288 Jul 25, 2022
mergify bot pushed a commit that referenced this issue Jul 25, 2022
@mtreinish @garrison
Fix handling of numpy arrays for indices in marginal_distribution (#8288
f868129 
)

* Fix handling of numpy arrays for indices in marginal_distribution

This commit fixes an issue with the marginal_distribution() function
where it would previously error if a numpy arrray of ints was passed in
for the indices parameter. This was caused by the input validation
looking for empty input by checking `not indices` which isn't valid for
numpy arrays. This commit fixes this by adjusting the check for
empty lists to work with any sequence input.

Fixes #8283

* Update releasenotes/notes/fix-numpy-indices-marginal-dist-45889e49ba337d84.yaml

Co-authored-by: Jim Garrison <jim@garrison.cc>

Co-authored-by: Jim Garrison <jim@garrison.cc>
@mergify mergify bot moved this to Done in Contributor Monitoring Jul 25, 2022
mergify bot pushed a commit that referenced this issue Jul 25, 2022
@mtreinish @mergify
Fix handling of numpy arrays for indices in marginal_distribution (#8288
eea6a7a 
)

* Fix handling of numpy arrays for indices in marginal_distribution

This commit fixes an issue with the marginal_distribution() function
where it would previously error if a numpy arrray of ints was passed in
for the indices parameter. This was caused by the input validation
looking for empty input by checking `not indices` which isn't valid for
numpy arrays. This commit fixes this by adjusting the check for
empty lists to work with any sequence input.

Fixes #8283

* Update releasenotes/notes/fix-numpy-indices-marginal-dist-45889e49ba337d84.yaml

Co-authored-by: Jim Garrison <jim@garrison.cc>

Co-authored-by: Jim Garrison <jim@garrison.cc>
(cherry picked from commit f868129)
mergify bot added a commit that referenced this issue Jul 25, 2022
@mergify @mtreinish
Fix handling of numpy arrays for indices in marginal_distribution (#8288
1dd3444 
) (#8396)

* Fix handling of numpy arrays for indices in marginal_distribution

This commit fixes an issue with the marginal_distribution() function
where it would previously error if a numpy arrray of ints was passed in
for the indices parameter. This was caused by the input validation
looking for empty input by checking `not indices` which isn't valid for
numpy arrays. This commit fixes this by adjusting the check for
empty lists to work with any sequence input.

Fixes #8283

* Update releasenotes/notes/fix-numpy-indices-marginal-dist-45889e49ba337d84.yaml

Co-authored-by: Jim Garrison <jim@garrison.cc>

Co-authored-by: Jim Garrison <jim@garrison.cc>
(cherry picked from commit f868129)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Contributor Monitoring
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix handling of numpy arrays for indices in marginal_distribution mtreinish/qiskit-core
3 participants
@nonhermitian @mtreinish @Cryoris