qiboteam / qibo

A framework for quantum computing
https://qibo.science
Apache License 2.0
287 stars 58 forks source link

SABRE + CircuitMap Optimization #1419

Closed csookim closed 1 week ago

csookim commented 1 month ago

To improve Sabre's performance, several implementations have been updated. I’ve added comments to the code and each comment number corresponds to the explanation numbers provided below. Please let me know if any clarifications are needed. Below is the code explanation.

Checklist:

After the code review:

csookim commented 1 month ago

SABRE + CircuitMap Optimization

Previous Implementation

New Implementation

The code has been updated in three key areas:

1. Qubit Mapping

1-1. Layout Relabel

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L731-L739

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L702-L707

1-2 Physical-Logical Mapping

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L183-L192

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L280-L284

2. Temporary CircuitMap

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L194-L196

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L843-L849

3. Blocks in the front_layer

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L990-L995

self._dag = [(0, {'qubits': (0, 9), 'layer': 0}), (1, {'qubits': (5, 9), 'layer': 1}), (2, {'qubits': (2, 8), 'layer': 0})]

https://github.com/qiboteam/qibo/blob/0111a6e49bc27b8e5b8102f5eb6615a093ade35c/src/qibo/transpiler/router.py#L807-L815

4. Additional Improvements

Evaluation

                     Mean   Std
Qiskit Sabre Time    0.0439 0.0075
Qibo Sabre Time      0.0625 0.0179
Qibo Sabre Old Time  0.4252 0.106

Qiskit Sabre Swaps   189.97 21.7295
Qibo Sabre Swaps     196.54 21.8712
Qibo Sabre Old Swaps 279.96 35.739
Screenshot 2024-08-14 at 20 49 23
codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.11%. Comparing base (434989e) to head (24ecc7a). Report is 26 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1419 +/- ## ======================================= Coverage 97.10% 97.11% ======================================= Files 81 81 Lines 11653 11679 +26 ======================================= + Hits 11316 11342 +26 Misses 337 337 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibo/pull/1419/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibo/pull/1419/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `97.11% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

csookim commented 1 month ago

Custom qubit names can be used in Sabre. But the node names in the connectivity graph and the physical qubit names in the layout must be identical. Layout relabeling and recovery are done within CircuitMap.

CircuitMap

Regarding the test files, some placers currently use node names that do not match the physical qubit names.

https://github.com/qiboteam/qibo/blob/21732ad59d31740e8c0641921abe98e49a1ea8c1/src/qibo/transpiler/placer.py#L203-L205

I believe some parts of the code in Placer need to be updated, and the test files should be updated after resolving issue #1429.

BrunoLiegiBastonLiegi commented 4 weeks ago

Thanks @csookim for the insight about the wire_names, indeed as this PR is already adding various optimizations to the CircuitMap object, I am ok with merging this with the support for the default naming scheme only (q0, q1, ...), as long as we open an issue to remind us about it. So, in the end, up to you, if you feel this is something straightforward we could address it here directly, fine, otherwise we leave it for a future PR. In any case the support to custom qubit names is probably not a widely used feature anyway...

Simone-Bordoni commented 4 weeks ago

Custom qubit names can be used in Sabre. But the node names in the connectivity graph and the physical qubit names in the layout must be identical. Layout relabeling and recovery are done within CircuitMap.

CircuitMap

  • Receive the layout {'A': 2, 'B': 0, 'C': 1}.
  • Assign physical qubit numbers to the keys of the layout ('A' -> 0, 'B' -> 1, 'C' -> 2).
  • Relabel the connectivity graph based on this mapping.
  • Save the qubit names (keys) in wire_names.
  • Logical qubit numbers are the values of the layout.
  • Route using physical and logical qubit numbers.
  • Restore the original connectivity graph using the inverse of the mapping (0 -> 'A', 1 -> 'B', 2 -> 'C').

Regarding the test files, some placers currently use node names that do not match the physical qubit names.

https://github.com/qiboteam/qibo/blob/21732ad59d31740e8c0641921abe98e49a1ea8c1/src/qibo/transpiler/placer.py#L203-L205

I believe some parts of the code in Placer need to be updated, and the test files should be updated after resolving issue #1429.

I agree on relabelling the graph nodes with the physical qubit names. However, in principle, the two objects should already have the same names. It is better to raise at least a warning when the names do not match, something like "Connectivity graph nodes names and physical qubit names dont match. Renaming connectivity nodes after physical qubits."

Simone-Bordoni commented 4 weeks ago

Thanks @csookim for the insight about the wire_names, indeed as this PR is already adding various optimizations to the CircuitMap object, I am ok with merging this with the support for the default naming scheme only (q0, q1, ...), as long as we open an issue to remind us about it. So, in the end, up to you, if you feel this is something straightforward we could address it here directly, fine, otherwise we leave it for a future PR. In any case the support to custom qubit names is probably not a widely used feature anyway...

At the moment it is not an important requirement but it may become fundamental for qibolab in the future when it will support hardware qubit names different from integers.

Simone-Bordoni commented 4 weeks ago

@csookim there are a few tests failing in the pipeline and router. If you can fix them then i will proceed with the review

csookim commented 4 weeks ago

Thanks for your review @Simone-Bordoni @BrunoLiegiBastonLiegi.

I agree on relabelling the graph nodes with the physical qubit names. However, in principle, the two objects should already have the same names. It is better to raise at least a warning when the names do not match, something like "Connectivity graph nodes names and physical qubit names dont match. Renaming connectivity nodes after physical qubits."

Relabeling is used for Sabre optimization. Sabre receives the layout as {'A': 2, 'B': 0, 'C': 1} and the connectivity graph as A-B-C. These names are both converted to numerical values for faster routing: 'A' -> 0, 'B' -> 1, 'C' -> 2.

I believe that the name matching/check should already occur at the Placer level.

@csookim there are a few tests failing in the pipeline and router. If you can fix them then i will proceed with the review

Yes, but to address this, the Placer should be updated first to enable same names for connectivity graph nodes and physical qubits.

I'll work on updating Placer and the test files.

@BrunoLiegiBastonLiegi Could you delete the reviews that you think have already been addressed?

BrunoLiegiBastonLiegi commented 4 weeks ago

I just did, there are some open comments that were not addressed and did not receive any answer

BrunoLiegiBastonLiegi commented 3 weeks ago

@csookim I don't see any new updates or answers to the points that are still open, could you please double check those and the tests that are failing? Regarding the custom wires_names, as agreed, we could disentagle that from this PR and address it in a separate one, just please open an issue about that as a reminder.

csookim commented 3 weeks ago

@BrunoLiegiBastonLiegi Could you close these reviews if resolved? If you need further clarification, please let me know.

1

@csookim do you agree that the circuit is not really optional or am I missing something?

2

Any comment about this?

looking at the docstring above for initial_layout, it seems that the keys are the logical indices and the values the physical ones, whereas here is reverted, am I missing something? Furthermore this snippet here seems to be more or less replaced by the set_p2l function below called on initial_layout.

3

this is not super important, but still I would prefer to get rid of this ambiguity

BrunoLiegiBastonLiegi commented 3 weeks ago

@BrunoLiegiBastonLiegi Could you close these reviews if resolved? If you need further clarification, please let me know.

I cannot see any answer to those comments, did you answer them? Please double check, because if you click the button "start a review" instead of "Add a single comment", your answers are going to be pending until you finish the review, and they are going to be visible for you only.

csookim commented 3 weeks ago

@BrunoLiegiBastonLiegi Could you close these reviews if resolved? If you need further clarification, please let me know.

I cannot see any answer to those comments, did you answer them? Please double check, because if you click the button "start a review" instead of "Add a single comment", your answers are going to be pending until you finish the review, and they are going to be visible for you only.

Oh, I didn't know I needed to submit my review. Sorry for the mistake.

csookim commented 3 weeks ago

@BrunoLiegiBastonLiegi I've updated the code handling these issues and modified the test functions.

Ah I see, then I believe it would be better to move the circuit check right after the temporary one:

        if self._temporary:  # if temporary circuit, no need to store the blocks
            return
        elif circuit is None:
            raise_error(ValueError, "Circuit must be provided for a non temporary `CircuitMap`.")
        self._nqubits = circuit.nqubits  # number of qubits

Except for the wire_names, I believe the case of the helper function is still open

Currently, the Router operates on physical qubits qi in the layout, which correspond to i in the connectivity nodes. Once all of your reviews are resolved, I will remove the comments.

Simone-Bordoni commented 2 weeks ago

I agree with the PR, However after the latest merge with master there is an error occurring. Can you fix it before I approve?

csookim commented 2 weeks ago

I agree with the PR, However after the latest merge with master there is an error occurring. Can you fix it before I approve?

There is a bug in drawer #1438. I will update it as soon as the issue is resolved. Please approve it after the update.

csookim commented 1 week ago

@Simone-Bordoni Please review this PR. Thanks.