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

Remove a validation check of circuits for BaseSampler (follow-up of #8678) #8708

Merged
merged 6 commits into from
Sep 29, 2022
Merged

Remove a validation check of circuits for BaseSampler (follow-up of #8678) #8708

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
195aefd
relax a check of measurements for Sampler
t-imamichi Sep 8, 2022
3fc20ae
Merge branch 'main' into followup-8678
t-imamichi Sep 16, 2022
1898e79
Merge branch 'main' into followup-8678
t-imamichi Sep 20, 2022
ebcf87b
Merge branch 'main' into followup-8678
t-imamichi Sep 27, 2022
71ee01a
fix a test
t-imamichi Sep 27, 2022
be3f4e3
Merge branch 'main' into followup-8678
mergify[bot] Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions qiskit/primitives/base_sampler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
from qiskit.utils.deprecation import deprecate_arguments, deprecate_function

from .sampler_result import SamplerResult
from .utils import _circuit_key, final_measurement_mapping
from .utils import _circuit_key


class BaseSampler(ABC):
Expand Down Expand Up @@ -393,14 +393,6 @@ def run(
"on the desired qubits."
)

mapping = final_measurement_mapping(circuit)
if set(range(circuit.num_clbits)) != set(mapping.values()):
raise QiskitError(
f"Some classical bits of the {i}-th circuit are not used for measurements."
f" the number of classical bits ({circuit.num_clbits}),"
f" the used classical bits ({set(mapping.values())})."
)

run_opts = copy(self.options)
run_opts.update_options(**run_options)

Expand Down
6 changes: 6 additions & 0 deletions qiskit/primitives/sampler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ def _run(
def _preprocess_circuit(circuit: QuantumCircuit):
circuit = init_circuit(circuit)
q_c_mapping = final_measurement_mapping(circuit)
if set(range(circuit.num_clbits)) != set(q_c_mapping.values()):
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it is still going to raise if the number of clbits is not equal to the number of final measurements. That is to say that if I wrote to all my bits via mid-circuit measurements then this would be triggered.

Copy link
Member Author

@t-imamichi t-imamichi Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is part of the reference implementation based on quantum_info.Statevector. It requires final measurements. Statevector does not support mid-circuit measurements.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the title of this PR to mention BaseSampler

Copy link
Contributor

Choose a reason for hiding this comment

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

But then you are blocking real-world use cases for a simulation method that does not scale. Does not seem like a fruitful direction to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reference implementation was designed to be as simple as possible. Runtime primitives and Aer primitives are more appropriate for scaling, e.g., Qiskit/qiskit-aer#1531
This is similar to the relationship of BasicAer and Aer simulator. The reference implementation of primitives corresponds to BasicAer and Aer primitives corresponds to Aer simulator.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is another way to think about it; Just overload all in Runtime. In some sense that is what would probably need to be done because a state vector simulator is quite far removed from a real hardware sampling routine

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a PR #8668 working on backend-based Sampler. It is closer to real hardware sampling. Once it's merged, we can discuss whether we need the reference implementation.

Copy link
Member Author

@t-imamichi t-imamichi Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This check is only part of the reference implementation of Sampler and not part of BaseSampler. So, other Sampler implementations does use this check. Other implementations are supposed to inherit only BaseSampler.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reference implementation Sampler needs this check. So, I have to put it in BaseSampler (abstract class) or Sampler (reference implementation). I moved it from Sampler to BaseSampler in #8668. I want to revert it in this PR. If this PR is not merged, the check stays in BaseSampler and raises an error for all implementations not only the reference impl but also qiskit-ibm-runtime.

raise QiskitError(
"Some classical bits are not used for measurements."
f" the number of classical bits ({circuit.num_clbits}),"
f" the used classical bits ({set(q_c_mapping.values())})."
)
c_q_mapping = sorted((c, q) for q, c in q_c_mapping.items())
qargs = [q for _, q in c_q_mapping]
circuit = circuit.remove_final_measurements(inplace=False)
Expand Down
5 changes: 2 additions & 3 deletions releasenotes/notes/add-sampler-error-check-38426fb186db44d4.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
---
upgrade:
- |
Added some error checks to :meth:`~qiskit.primitives.BaseSampler.run`.
It raises an error if there is no classical bit or some classical bits
are not used for measurements.
Added a validation check to :meth:`~qiskit.primitives.BaseSampler.run`.
It raises an error if there is no classical bit.