Support contiguous qubits for OpenQASM IR #237

yitchen-tim · 2024-04-05T15:25:00Z

Description of changes:

Qubits are mapped to contiguous qubits before simulation
Adapt the indices of measuredQubit in the returned simulation result

Testing done:
tox

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2024-04-05T15:27:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (420aeaf) to head (1b9fdcf).
Report is 28 commits behind head on main.

❗ Current head 1b9fdcf differs from pull request most recent head ae762a6

Please upload reports for the commit ae762a6 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #237   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        48           
  Lines         3770      3798   +28     
  Branches       915       925   +10     
=========================================
+ Hits          3770      3798   +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

laurencap · 2024-04-08T14:48:06Z

src/braket/default_simulator/simulator.py

-            measuredQubits=(
-                measured_qubits if measured_qubits else self._get_all_qubits(simulation.qubit_count)
-            ),
+            measuredQubits=(measured_qubits if measured_qubits else used_qubits),


Does the order have an effect? Does it align with classical bit indices?

This is used only in the case that there's no requested measurement, and changes the default behavior to measure all used qubits rather than all declared qubits?

Does the order have an effect? Does it align with classical bit indices?

This must be the sorted order of the used qubits, right?

laurencap · 2024-04-08T15:00:48Z

test/unit_tests/braket/default_simulator/test_density_matrix_simulator.py

@@ -301,7 +301,7 @@ def test_properties():
                    ],
                    "supportPhysicalQubits": False,
                    "supportsPartialVerbatimBox": False,
-                    "requiresContiguousQubitIndices": True,
+                    "requiresContiguousQubitIndices": False,


I wonder why the test properties don't use the actual properties?

laurencap · 2024-04-08T15:08:15Z

test/unit_tests/braket/default_simulator/test_density_matrix_simulator.py

@@ -795,3 +795,21 @@ def test_measure_targets():
    assert 400 < np.sum(measurements, axis=0)[0] < 600
    assert len(measurements[0]) == 1
    assert result.measuredQubits == [0]
+
+
+def test_non_contiguous_qubits_with_shots0():


Can you please add tests showing returning measurements for multiple qubits? Does the ordering follow the user's order, or is it sorted by the qubit index?

Can you also add tests with sampled results? I think that's the kind of result that _formatted_measurements acts on, is that right?

laurencap · 2024-04-08T15:14:24Z

src/braket/default_simulator/simulator.py

@@ -438,8 +475,12 @@ def run_openqasm(
        """
        circuit = self.parse_program(openqasm_ir).circuit
        qubit_count = circuit.num_qubits
+        used_qubits = sorted(list(circuit.qubit_set))


I prefer it when variables are defined closer to where they are used. That requires less jumping around the code to figure out what a variable means, and less "mental mapping" where I have to store a lot of info in my head while reading a function. Since this is only used once, you don't even have to define it at all, and can just pass it to the function that needs it.

If you do define it here, why not use it for line 481? used_qubits[-1] would be faster than max(set).

If you keep the variable, could we call it sorted_used_qubits? It's a bit verbose, but it was my first question when I saw this variable name elsewhere.

laurencap · 2024-04-08T15:20:38Z

src/braket/default_simulator/simulator.py

@@ -479,7 +520,9 @@ def run_openqasm(
        else:
            simulation.evolve(circuit.basis_rotation_instructions)

-        return self._create_results_obj(results, openqasm_ir, simulation, measured_qubits)
+        return self._create_results_obj(
+            results, openqasm_ir, simulation, used_qubits, measured_qubits


What if you moved the measured_qubits if measured_qubits else used_qubits logic here?

It looks to me at the moment that _create_results_obj won't ever need both of them -- and the used_qubits simply are the measured qubits when measured qubits is None. This has the extra benefit of reducing the number of variables passed to that function, making it a little easier to understand what the function does.

laurencap · 2024-04-08T15:32:28Z

src/braket/default_simulator/simulator.py

        measured_qubits = circuit.measured_qubits

+        if max(circuit.qubit_set) != len(circuit.qubit_set) - 1:
+            circuit = self._map_to_contiguous_qubits(circuit)


Maybe _map_circuit_to_contiguous_qubits? The name as is makes me expect indices as inputs. Note how similar, semantically, this name is to _contiguous_qubit_mapping. They're synonyms.

Another nit: Can we move line 481 into this function? _formatted_measurements also containerizes this check.

I prefer it because it makes it clear that every circuit will be mapped to operate on contiguous qubits. It just so happens that some circuits already trivially do this, but that fact might be distracting here -- a cursory look might make someone think we're only conditionally getting contiguous qubits

laurencap · 2024-04-08T15:34:05Z

src/braket/default_simulator/simulator.py

+        """Map the qubits in operations and result types to contiguous qubits.
+
+        Args:
+            circuit (Circuit): Circuit which qubits are not contiguous.


Suggested change

circuit (Circuit): Circuit which qubits are not contiguous.

circuit (Circuit): Circuit in which qubits may not be contiguous.

Or simply, Circuit to map.

laurencap · 2024-04-08T15:34:15Z

src/braket/default_simulator/simulator.py

+            circuit (Circuit): Circuit which qubits are not contiguous.
+
+        Returns:
+            Circuit: Circuit qubits mapped to with contiguous qubits.


Suggested change

Circuit: Circuit qubits mapped to with contiguous qubits.

Circuit: Circuit with qubits mapped to contiguous qubits.

laurencap · 2024-04-08T15:35:09Z

src/braket/default_simulator/simulator.py

        return measurements

+    def _map_to_contiguous_qubits(self, circuit: Circuit) -> Circuit:
+        """Map the qubits in operations and result types to contiguous qubits.


It would be nice to include here a Note: describing what contiguous qubits are, explaining that simulators require them for computation, and we handle the user's requested mapping here (which is useful for running on QPUs, that might be TMI though, idk)

laurencap · 2024-04-08T15:36:20Z

src/braket/default_simulator/simulator.py

-            measurements = np.array(measurements)[:, measured_qubits].tolist()
+            if max(measured_qubits) != len(measured_qubits) - 1:
+                qubit_map = BaseLocalSimulator._contiguous_qubit_mapping(measured_qubits)
+                mapped_measured_qubits = [qubit_map[q] for q in measured_qubits]


is mapped_measured_qubits ever different from the inverse of qubit_map?

map to contiguous qubits before simulation

45c1940

add unit tests for dm simulator

1b9fdcf

yitchen-tim marked this pull request as ready for review April 5, 2024 15:49

yitchen-tim requested a review from a team as a code owner April 5, 2024 15:49

yitchen-tim marked this pull request as draft April 5, 2024 16:58

laurencap requested changes Apr 8, 2024

View reviewed changes

translate result types before remapping

ae762a6

rmshaffer mentioned this pull request May 21, 2024

Support non-contiguous qubit indices #252

Closed

yitchen-tim closed this Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support contiguous qubits for OpenQASM IR #237

Support contiguous qubits for OpenQASM IR #237

yitchen-tim commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

laurencap Apr 8, 2024

	circuit (Circuit): Circuit which qubits are not contiguous.
	circuit (Circuit): Circuit in which qubits may not be contiguous.

	Circuit: Circuit qubits mapped to with contiguous qubits.
	Circuit: Circuit with qubits mapped to contiguous qubits.

Support contiguous qubits for OpenQASM IR #237

Support contiguous qubits for OpenQASM IR #237

Conversation

yitchen-tim commented Apr 5, 2024 • edited Loading

Merge Checklist

General

Tests

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitchen-tim commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading