Skip to content

fixes #11393 sampled_expectation_value returning incorrect results for complex coefficients (gave 0.0 rather than 1j)#16443

Open
Yosef-Bunick wants to merge 10 commits into
Qiskit:mainfrom
Yosef-Bunick:fix-sampled-expval-complex-11393
Open

fixes #11393 sampled_expectation_value returning incorrect results for complex coefficients (gave 0.0 rather than 1j)#16443
Yosef-Bunick wants to merge 10 commits into
Qiskit:mainfrom
Yosef-Bunick:fix-sampled-expval-complex-11393

Conversation

@Yosef-Bunick

@Yosef-Bunick Yosef-Bunick commented Jun 17, 2026

Copy link
Copy Markdown

fixes #11393
partially fixes #16443
Root cause: two Rust functions computed Complex64 but returned Ok(.re).
PR #11471 got half of it,
they got sampled_expval_complex (.re → Ok)
but missed the sampled_expval_sparse_observable (.re → Ok)
also changed the "bit_array.py dtype + note" and added a few tests

AI/LLM disclosure

  • I used the following tool to generate or modify code: ABE_v4 (personal custom AI engine)

@Yosef-Bunick Yosef-Bunick requested review from a team as code owners June 17, 2026 05:20
@Yosef-Bunick Yosef-Bunick requested a review from raynelfss June 17, 2026 05:20
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 17, 2026
@qiskit-bot

Copy link
Copy Markdown
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

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

@CLAassistant

CLAassistant commented Jun 17, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Yosef-Bunick

Yosef-Bunick commented Jun 17, 2026

Copy link
Copy Markdown
Author

Fixes #11393

@mergify

mergify Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

queue

☑️ Command disallowed due to command restrictions in the Mergify configuration.

Details
  • sender-permission >= write

@ShellyGarion

Copy link
Copy Markdown
Member

@Yosef-Bunick - Thank you for your contribution to Qiskit!
Could you please clarify whether this PR was authored purely using AI tools, or if you have reviewed and validated the changes yourself? We ask contributors to follow the guidelines on the use of AI tools, as described here: Qiskit Contribution Guidelines – Use of AI tools.
Please also note that issues labeled “good first issue” are primarily intended to support learning and development for human contributors and not AI tools.
Additionally, if this PR builds upon work by another contributor (e.g., #11471), could you please ensure they are properly credited, for example by adding them as a co-author?

Comment thread qiskit/primitives/containers/bit_array.py Outdated
@Yosef-Bunick

Copy link
Copy Markdown
Author

@ShellyGarion
Yes I've read and reviewed the whole thing, and tried backtracking the issue.
The main issue with the first pull was that they tried changing it to complex64 formatting,
but they didn't change it on the return out.re->out and in python the equivalency is complex128.
If you would like me to add them as a coauthor, please advise how to add zeb33n.
If this one is good, I will try a harder one.

@ShellyGarion

Copy link
Copy Markdown
Member

@ShellyGarion Yes I've read and reviewed the whole thing, and tried backtracking the issue. The main issue with the first pull was that they tried changing it to complex64 formatting, but they didn't change it on the return out.re->out and in python the equivalency is complex128. If you would like me to add them as a coauthor, please advise how to add zeb33n. If this one is good, I will try a harder one.

If possible, you can add them by a git commit co-authored-by.
If this doesn't work, just mention it in the PR decryption above.

@Yosef-Bunick Yosef-Bunick force-pushed the fix-sampled-expval-complex-11393 branch 2 times, most recently from 31aa117 to e23b663 Compare June 17, 2026 23:53
@Yosef-Bunick

Copy link
Copy Markdown
Author

@ShellyGarion Yes I've read and reviewed the whole thing, and tried backtracking the issue. The main issue with the first pull was that they tried changing it to complex64 formatting, but they didn't change it on the return out.re->out and in python the equivalency is complex128. If you would like me to add them as a coauthor, please advise how to add zeb33n. If this one is good, I will try a harder one.

If possible, you can add them by a git commit co-authored-by. If this doesn't work, just mention it in the PR decryption above.

@ShellyGarion
Added test and added co-authored by zeb33n.
Lmk if it looks right.

@Yosef-Bunick Yosef-Bunick changed the title fixes #11393 (sampled_expectation_value dropping imaginary part) fixes #11393 (expanded upon zeb33n's #11471 Ok(out.re) ) Jun 18, 2026
@ShellyGarion ShellyGarion added Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. mod: primitives Related to the Primitives module labels Jun 18, 2026
@ShellyGarion ShellyGarion requested a review from ihincks June 18, 2026 08:42
@ShellyGarion

Copy link
Copy Markdown
Member

it seems that there are formatting errors and tests failures. please check this.

Comment thread qiskit/primitives/containers/bit_array.py
Comment thread test/python/primitives/containers/test_bit_array.py Outdated
@Yosef-Bunick

Yosef-Bunick commented Jun 18, 2026

Copy link
Copy Markdown
Author

@ShellyGarion
should be good to go, fixed the -1 error, (mistake in test)
changed the lint error, ok(result?)->result

@ihincks changed to arr->np.real_if_close(arr) and
[np.complex128]:->[np.float64 | np.complex128]:
to support it.

I got All passing on tests localy

    sampled_expval_complex and sampled_expval_sparse_observable computed
    the full Complex64 result but returned only Ok(.re). Complex coefficients
    (e.g. SparsePauliOp with -1j) produced 0.0 instead of 1j.

    - crates/accelerate: return Complex64, not f64 (2 functions)
    - sampled_expval.py: return float | complex, updated docstring
    - bit_array.py: dtype=complex128, removed misleading note
    - test: 5 regression tests across both modules
    - releasenotes: release note

    Co-authored-by: zeb33n <zeb33n@users.noreply.github.com>
    Fixes Qiskit#11393
@Yosef-Bunick Yosef-Bunick force-pushed the fix-sampled-expval-complex-11393 branch from 969fc65 to 664ded1 Compare June 18, 2026 20:23
@Yosef-Bunick Yosef-Bunick changed the title fixes #11393 (expanded upon zeb33n's #11471 Ok(out.re) ) fixes #11393 (expanded upon zeb33n's #11471 Ok(out.re) and according to ihincks np.real_if_close recomendation) Jun 18, 2026
@Yosef-Bunick Yosef-Bunick force-pushed the fix-sampled-expval-complex-11393 branch from 6b8612c to 664ded1 Compare June 19, 2026 04:41
Comment thread releasenotes/notes/fix-sampled-expval-complex-11393.yaml Outdated
Comment thread test/python/primitives/containers/test_bit_array.py Outdated
Comment thread test/python/result/test_sampled_expval.py Outdated

@Cryoris Cryoris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we want to expose a tolerance to cutoff the imaginary part, np.real_if_close does support this. We can leave this as a follow up though.

If the test concerns I left are addressed this LGTM.

…and ammended one test verify real comes back real, and complex comes back complex.
@Yosef-Bunick Yosef-Bunick force-pushed the fix-sampled-expval-complex-11393 branch from 4c32409 to 90a12ed Compare June 24, 2026 00:16
Yosef-Bunick and others added 3 commits June 23, 2026 20:23
changes to complex or float here, instead of waiting for the return

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

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

Yosef-Bunick commented Jun 24, 2026

Copy link
Copy Markdown
Author

I'm wondering if we want to expose a tolerance to cutoff the imaginary part, np.real_if_close does support this. We can leave this as a follow up though.

If the test concerns I left are addressed this LGTM.

@Cryoris
prob more ideal to put 'non default tol' in separate pull in case the 'non default tol' isn't appealing and can be reverted easily.

fixed the docstrings that didn't match what was actually being tested,
added assertNotIsInstance(result, complex) guards on both real-coefficient paths so a missing np.real_if_close gets caught.

should be ready to push, passed the lint, black, and tests localy.

@Yosef-Bunick Yosef-Bunick changed the title fixes #11393 (expanded upon zeb33n's #11471 Ok(out.re) and according to ihincks np.real_if_close recomendation) fixes #11393 (expanded upon @zeb33n's #11471 Ok(out.re) , @ihincks np.real_if_close ,and according to @Cryoris " NDArray[ " recommendation ) Jun 24, 2026
@Yosef-Bunick

Yosef-Bunick commented Jun 24, 2026

Copy link
Copy Markdown
Author

I'm wondering if we want to expose a tolerance to cutoff the imaginary part, np.real_if_close does support this. We can leave this as a follow up though.

reference for separate pull
{
details for tol fix reference, might need different hookup depending on where you want it hooked up.
qiskit/result/sampled_expval.py: approximately line 40 "tol: float = 100," 75 "tol=tol", 90 "tol=tol"
qiskit/primitives/containers/bit_array.py: approximately line 616 "tol: float = 100," 650 "tol=tol", 654 "tol=tol"
and add test.
}

Comment thread test/python/primitives/containers/test_bit_array.py Outdated
Comment thread qiskit/result/sampled_expval.py Outdated
@Cryoris

Cryoris commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@Yosef-Bunick The PR title is getting out of hand, please just use a short descriptive title of what this is fixing.

complex->complex-valued for clarity

Co-authored-by: Julien Gacon <gaconju@gmail.com>
@Yosef-Bunick Yosef-Bunick changed the title fixes #11393 (expanded upon @zeb33n's #11471 Ok(out.re) , @ihincks np.real_if_close ,and according to @Cryoris " NDArray[ " recommendation ) fixes #11393 sampled_expectation_value returning incorrect results for complex coefficients (gave 0.0 rather than 1j) Jun 25, 2026
Updated the fix description for sampled expectation value functions to clarify the change in return types.
Update expectation_values method to always return float64 dtype.
internal complex because gate blocks it anyway in ObservablesArray.coerce(observables)
Yosef-Bunick added a commit to Yosef-Bunick/qiskit that referenced this pull request Jun 26, 2026
Yosef-Bunick added a commit to Yosef-Bunick/qiskit that referenced this pull request Jun 26, 2026
Yosef-Bunick added a commit to Yosef-Bunick/qiskit that referenced this pull request Jun 26, 2026
Yosef-Bunick added a commit to Yosef-Bunick/qiskit that referenced this pull request Jun 26, 2026
Yosef-Bunick added a commit to Yosef-Bunick/qiskit that referenced this pull request Jun 26, 2026
@Yosef-Bunick

Copy link
Copy Markdown
Author

Now, this pull only fixes sampled_expectation_value returning incorrect results for complex coefficients.
Moved bit_array.py to a separate pull request since they are independent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: primitives Related to the Primitives module

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

sampled_expectation_value returning incorrect results for complex coefficients

6 participants