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

reduce decimal for probabilities_dict in Sampler #10596

Merged
merged 11 commits into from
Sep 28, 2023

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Aug 9, 2023

The example in #10584 is:

import numpy as np
from qiskit import QuantumCircuit

# 1. A quantum circuit for preparing the quantum state |000> + i |111>
qc_example = QuantumCircuit(3)
qc_example.h(0) # generate superpostion
qc_example.cx(0,1) # condition 1st qubit on 0th qubit
qc_example.cx(0,2) # condition 2nd qubit on 0th qubit

qc_measured = qc_example.measure_all(inplace=False)

# 3. Execute using the Sampler primitive
from qiskit.primitives.sampler import Sampler
sampler = Sampler()
job = sampler.run(qc_measured)
result = job.result()
print("> Quasi probability distribution:", result.quasi_dists)
> Quasi probability distribution: [{0: 0.4999999999999999, 7: 0.4999999999999999}]

The rounding error is coming from slightly high np.round(..., decimals=16), original introduced in #9248. Moving that to 15 solved the issue.

> Quasi probability distribution: [{0: 0.5, 7: 0.5}]

Not sure if reno-worthy Actually, it might change the output of QAOA, so adding a update note.

@1ucian0 1ucian0 requested review from ikkoham, t-imamichi and a team as code owners August 9, 2023 15:11
@qiskit-bot
Copy link
Collaborator

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

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5822445480

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 16 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.008%) to 87.242%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
crates/qasm2/src/lex.rs 3 91.14%
crates/qasm2/src/parse.rs 12 96.67%
Totals Coverage Status
Change from base Build 5820521776: -0.008%
Covered Lines: 74274
Relevant Lines: 85136

💛 - Coveralls

@Cryoris
Copy link
Contributor

Cryoris commented Aug 14, 2023

Since this seems to be mainly a visualization issue, would it be better to change the printed values instead of the actual values? Otherwise, as you noted, this small changes can add up 🤔

@1ucian0
Copy link
Member Author

1ucian0 commented Aug 28, 2023

Since this seems to be mainly a visualization issue, would it be better to change the printed values instead of the actual values? Otherwise, as you noted, this small changes can add up 🤔

Is there a way to "parametrize" the visualisation of a result?

@Cryoris
Copy link
Contributor

Cryoris commented Aug 29, 2023

Yeah e.g. via QuasiDist.__str__ 🙂

@1ucian0
Copy link
Member Author

1ucian0 commented Aug 31, 2023

Yeah e.g. via QuasiDist.__str__ 🙂

I mean, to tell how many decimals I want.

@Cryoris
Copy link
Contributor

Cryoris commented Sep 1, 2023

NumPy allows to set the precision of printed numbers, but I think this is only a global setting. With __str__ I meant you could do something like

# in QuasiDist.__str__
truncated = {key: np.round(value, decimals=10) for key, value in self.items}
return str(truncated)

@1ucian0
Copy link
Member Author

1ucian0 commented Sep 1, 2023

Sure... I'm still curious why changing the rounding in #9248 changes the output. But a fight for another day. The __repr__ approach implemented in 7787b8b

@@ -51,6 +51,7 @@ def __init__(self, data, shots=None, stddev_upper_bound=None):
self.shots = shots
self._stddev_upper_bound = stddev_upper_bound
self._num_bits = 0
self.decimal = 15
Copy link
Member

@t-imamichi t-imamichi Sep 4, 2023

Choose a reason for hiding this comment

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

It might be good to set the decimal with a class variable __decimal__ or __ndigits__ (instead of instance variable self.decimal) similar to Pauli.__repr__.

Using ``str`` to convert a ``Pauli`` to a string will truncate the
returned string for large numbers of qubits while :meth:`to_label`
will return the full string with no truncation. The default
truncation length is 50 characters. The default value can be
changed by setting the class `__truncate__` attribute to an integer
value. If set to ``0`` no truncation will be performed.

Do you have any suggestion, @ikkoham?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @t-imamichi. Class variable is better than instance variable. Variable names can be something like dunder/magic (__decimal__), or they can be normal (decimal).

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea! done in d2866a3

Thanks!

@ikkoham ikkoham added mod: primitives Related to the Primitives module Changelog: None Do not include in changelog labels Sep 11, 2023
@1ucian0 1ucian0 added this to the 0.45.0 milestone Sep 27, 2023
@coveralls
Copy link

coveralls commented Sep 27, 2023

Pull Request Test Coverage Report for Build 6334811226

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • 20 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.009%) to 86.998%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/result/distributions/quasi.py 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 90.15%
crates/qasm2/src/parse.rs 18 96.67%
Totals Coverage Status
Change from base Build 6332583014: -0.009%
Covered Lines: 74079
Relevant Lines: 85150

💛 - Coveralls

Copy link
Member

@t-imamichi t-imamichi left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM.

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 too

@Cryoris Cryoris added this pull request to the merge queue Sep 28, 2023
Merged via the queue into Qiskit:main with commit e5420d2 Sep 28, 2023
rupeshknn pushed a commit to rupeshknn/qiskit that referenced this pull request Oct 9, 2023
* reduce decimal for probabilities_dict in Sampler

* adjust qaoa tests

* adjust qaoa tests

* reno

* Rolling everything back and just hook __repr__

Co-authored-by: Julien Gacon <[email protected]>

* remove reno

* QuasiDistribution.__repr__

---------

Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Takashi Imamichi <[email protected]>
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.

7 participants