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

Fixes for dependency issues caused by 0.14 release #2094

Merged
merged 23 commits into from
Apr 23, 2024

Conversation

doichanj
Copy link
Collaborator

@doichanj doichanj commented Apr 3, 2024

Summary

This PR fixed dependency issues caused by release 0.14.
Also this fix includes fix for #2084

Details and comments

  • fixed sampling measure > 63 qubits
  • ccz was accepted when input backend does not have ccz
  • fixed smaller coupling map was generated in Primitives V1
  • Remove skipping Python 3.8
  • Hide primitives V2 for Qiskit < 1.0 setup.py requirements for qiskit are outdated #2096

@doichanj doichanj added stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable Changelog: Bugfix Include in the Fixed section of the changelog labels Apr 3, 2024
@doichanj doichanj requested a review from hhorii April 4, 2024 06:31
Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

Are there any python unit tests for large Pauli's on the stabilizer simulator? If not you should add some (or if so add some more).

A basic one which is similar to the test I saw fail on 0.14 for my repo is something like preparing +1 Pauli eigenstates, measuring them, and checking the expectation value computed from the final counts is close to 1

Eg

    @ddt.data(65, 127, 433)
    def test_expectation_value_large_paulis(self, num_qubits):
        """Test expectation value circuit with large Pauli objects"""
        paulis = qi.PauliList([num_qubits * i for i in ["X", "Y", "Z"]])
        circuits = []
        for pauli in paulis:
            qc = QuantumCircuit(num_qubits, num_qubits)
            for i, p in enumerate(pauli):
                if p == Pauli("Y"):
                    qc.sdg(i)
                    qc.h(i)
                elif p == Pauli("X"):
                     qc.h(i)
                if p != Pauli("I"):
                    qc.measure(i, i)
        
        # Run circuits
        ...
        
        # Check all expvals are close to 1 where each expval is computed
        #  expval = sum_i freq_i * (1 - 2 * parity(bitsting_i)) / shots

@doichanj
Copy link
Collaborator Author

doichanj commented Apr 10, 2024

@chriseclectic I tested with the circuit below but both non-fixed version and this PR returns 1.0

        paulis = qi.PauliList([num_qubits * i for i in ["X", "Y", "Z"]])
        qc = QuantumCircuit(num_qubits, num_qubits)
        for pauli in paulis:
            for i, p in enumerate(pauli):
                if p == qi.Pauli("Y"):
                    qc.sdg(i)
                    qc.h(i)
                elif p == qi.Pauli("X"):
                     qc.h(i)
                if p != qi.Pauli("I"):
                    qc.measure(i, i)
        qubits = list(range(num_qubits))
        ostr = "I"*num_qubits
        oper = qi.Pauli(ostr)
        backend = self.backend(method=method)
        label = "expval"
        qc.save_expectation_value(oper, qubits, label=label)
        result = backend.run(transpile(qc, backend, optimization_level=0), shots=1).result()
        self.assertTrue(result.success)
        simdata = result.data(0)
        self.assertIn(label, simdata)
        value = simdata[label]
        self.assertAlmostEqual(value, 1.0)

something wrong with my code?
And I noticed that only randomize_measurement=True cases failed in pec_runtime test case

@chriseclectic
Copy link
Member

chriseclectic commented Apr 10, 2024

For this test you need to specifically test the measure instruction, not the save_expectation_value.

I'm not sure why the failure only occurred when randomizing the output and correcting in post-processing. I did double check the with result metadata used in post-processing to undo the randomization with a fixed generation seed and it was the same both in 0.13 and 0.14, so that leads me to think the issue is the count data itself when randomizing.

One thing when randomizing is the stabilizer state being sampled from (which is a simple tensor product state) won't be symmetric over different qubits anymore, when without randomization for this test it was. You should be able to capture this without randomization by Paulis that mix X, Y, Z.

@doichanj doichanj mentioned this pull request Apr 15, 2024
@@ -912,6 +914,9 @@ def _assemble_op(
aer_circ.mark(qubits, params)
elif name == "qerror_loc":
aer_circ.set_qerror_loc(qubits, label if label else name, conditional_reg, aer_cond_expr)
elif basis_gates is not None and name in basis_gates:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aer C++ library supports only the following gates natively.

"ccx", "ccz", "cp", "cswap", "csx", "cx", "cy", "cz", "delay", "ecr", "h",
"id", "mcp", "mcphase", "mcr", "mcrx", "mcry", "mcrz", "mcswap", "mcsx",
"mcu", "mcu1", "mcu2", "mcu3", "mcx", "mcx_gray", "mcy", "mcz", "p", "r"

If basis_gates is not subset of the above set, simulation will fail. I think it is better to check name is in the above set.

Also, I guess backend.configuration().basis_gate is list and not set. basis_gates should be set because this is one of hot loops.

I think that at the beginning of assemble_circuit(), basis_gates should be re-created as a set (while reducing measure, reset and etc). If None is passed, we can use a default set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When AerSimulator is made by from_backend() given basis_gates does not contain all the gates Aer supports, Aer should return error

"""converts a list of Qiskit circuits into circuits mapped AER::Circuit

Args:
circuits: circuit(s) to be converted
basis_gates (list): supported gates to be converted
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mentioned the above, list should be converted to set here.

if num_qubits == 65:
ref_counts.update(
{
"00000010101011110100110101101101101100010011000110011100101001010": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to test whether stabilizer can work for large-qubit circuits, we can use GHZ and then validate result with more simpler counts.

@doichanj doichanj requested a review from hhorii April 23, 2024 02:14
@hhorii hhorii merged commit 2150983 into Qiskit:main Apr 23, 2024
38 checks passed
doichanj added a commit to doichanj/qiskit-aer that referenced this pull request Apr 25, 2024
* Fixes for dependency issues

* lint

* lint

* lint

* fix release note

* fix sampler

* fix sampler

* fix sampler

* fix sampler

* remove skip cp38

* hide primitives V2 for qiskit < 1.0

* lint

* add test case for sampling measure for large stabilizer circuit

* reduce warning

* replace test case for large stabilizer with GHZ circuit

* format

* format

* convert basis_gates from list to set

* fix assemble_circuits
hhorii pushed a commit that referenced this pull request Apr 30, 2024
* Fix AerCompiler to use custom pass manager to decompose control flow ops (#2095)

* Fix AerCompiler to use basis_gates built from input circuit

* use custom pass manager to decompose dontrol flow ops

* remove unused import

* Use sphinx.ext.linkcode for more precise source code links (#2101)

* switched links to using sphinx.ext.linkcode

* added release note

* added check for qiskit methods

* remove whitespace

* removed regex subsitution

Co-authored-by: Eric Arellano <[email protected]>

* Update tox.ini

Co-authored-by: Eric Arellano <[email protected]>

* made updates following Eric's review

* made final line more readable

* cast filename to str

* re-added regex sub

---------

Co-authored-by: Eric Arellano <[email protected]>

* Fix noise sampling on shot-branching (#2098)

* Fix noise sampling on shot-branching

* format

* fix runtime noise sampling

* remove copying branch

* fix batch GPU

* format

* set initial value to Op structure

* format

* format

* test

* test

* fix use of additional_ops.size()

* fix error

* fix remove_empty_branches

* test

* test

* test

* test

* Fixes for dependency issues caused by 0.14 release (#2094)

* Fixes for dependency issues

* lint

* lint

* lint

* fix release note

* fix sampler

* fix sampler

* fix sampler

* fix sampler

* remove skip cp38

* hide primitives V2 for qiskit < 1.0

* lint

* add test case for sampling measure for large stabilizer circuit

* reduce warning

* replace test case for large stabilizer with GHZ circuit

* format

* format

* convert basis_gates from list to set

* fix assemble_circuits

* resolve conflicts

* Replace example in README to using primitives (#2105)

* Replace example in README to using primitives

* upgrade python version to 3.10 for github actions

* fix 3.10

* fix 3.10

* upgrade python version to 3.10 for github actions

* skip 3.8 and 3.9 for MacOS arm64

* skip 3.8 and 3.9 for MacOS arm64

* skip 3.8 and 3.9 for MacOS arm64

* replace macos-latest with macos-13

* Revert "Replace example in README to using primitives"

This reverts commit b536563.

* Revert "Revert "Replace example in README to using primitives""

This reverts commit 807ac6f.

* manually merge upstream

* add example using noise model

* remove print(result)

* add release note and set versions

* Fix CI failures (#2106)

* upgrade github actions for macos_arm64 and windows

* skip pp38/39

* exclude pp

* fix doc

* test

* remove CIBW_TEST_SKIP for wheel

---------

Co-authored-by: melechlapson <[email protected]>
Co-authored-by: Eric Arellano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the Fixed section of the changelog stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants