Run circuits following circuit map

Edoardo-Pedicillo commented 3 months ago

[!WARNING] ~this is just an hot fix to test RB (thanks @Jacfomg), it is not intended to be merged as it is.~

This PR is related to qiboteam/qibo#1301

Checklist:

[ ] Reviewers confirm new code works as expected.
[x] Tests are passing.
[x] Coverage does not decrease.
[x] Documentation is updated.

alecandido commented 3 months ago

Is it required to have the qubit_map within Qibolab?

If possible, I'd rather keep it at the level of the circuit, and convert to sequential integers (0, 1, 2, ...) whenever you interact with Qibolab.

My idea is that these abstract names are just part of the UI, but they are meaningless internally. So, I would avoid spreading UI details throughout all packages...

andrea-pasquale commented 3 months ago

Is it required to have the qubit_map within Qibolab?

If possible, I'd rather keep it at the level of the circuit, and convert to sequential integers (0, 1, 2, ...) whenever you interact with Qibolab.

My idea is that these abstract names are just part of the UI, but they are meaningless internally. So, I would avoid spreading UI details throughout all packages...

The idea behind this PR is the possibility to run on specific hardware qubits. Currently you can do this just by allocating a circuit with nqubits=platform.nqubits. It would be nice to have the possibility to run for example on qubit n without creating a n+1 circuit.

alecandido commented 3 months ago

The idea behind this PR is the possibility to run on specific hardware qubits. Currently you can do this just by allocating a circuit with nqubits=platform.nqubits. It would be nice to have the possibility to run for example on qubit n without creating a n+1 circuit.

I believe this is just a limitation in the platform definition, that we could make more flexible. For the time being, I'd suggest creating another platform with just those qubits.

Later on, we could make a PlatformView, or something like that, to improve the qubits slicing. But within the view, I'd keep the serials trivial (0, 1, 2, ...), and just keep the mapping internal (like it happens with NumPy arrays, e.g.).

alecandido commented 3 months ago

(If the problem is at the Circuit level, and not that you're occupying the whole platform, or you'd like to restrict the platform runcard, then just use a circuit with n+1 qubits, at least for the time being: the passive qubits are harmless, they even take negligible resources)

Edoardo-Pedicillo commented 3 months ago

Let's keep the conversation in one place, this is my answer for your message in the issue https://github.com/qiboteam/qibolab/issues/865#issuecomment-2036693165

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 66.61%. Comparing base (a514441) to head (f267c7e).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #864 +/- ## ========================================== + Coverage 66.59% 66.61% +0.01% ========================================== Files 55 55 Lines 5942 5945 +3 ========================================== + Hits 3957 3960 +3 Misses 1985 1985 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/864/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/qibolab/pull/864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `66.61% <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.

Edoardo-Pedicillo commented 2 months ago

UPDATE:

With this implementation, since the mapping is defined at the level of the transpiler and not at the circuit level ( for example with circuit_wire), is not possible, for example, to run a list of single qubit circuits on different qubits, because the mapping { q_i : 0 } is not invertible (I think this could be generalize when the number of qubits in the circuits is less than the hardware ones) , so this will not solve entirely the problem of RB in Qibocal, but I still consider it an improvement.

stavros11 commented 2 months ago

UPDATE:

With this implementation, since the mapping is defined at the level of the transpiler and not at the circuit level ( for example with circuit_wire), is not possible, for example, to run a list of single qubit circuits on different qubits, because the mapping { q_i : 0 } is not invertible (I think this could be generalize when the number of qubits in the circuits is less than the hardware ones) , so this will not solve entirely the problem of RB in Qibocal, but I still consider it an improvement.

I haven't tried, but given the modification done here in the compiler you should be able to do

# define single-qubit circuit
circuit = Circuit(1)
circuit.add(...)

compiler = Compiler.default()
sequence, _ = compiler.compile(circuit, platform, {0: qubit})

and the sequence will be on any given qubit from the platform. Isn't that sufficient for the RB?

Edoardo-Pedicillo commented 2 months ago

I haven't tried, but given the modification done here in the compiler you should be able to do
# define single-qubit circuit
circuit = Circuit(1)
circuit.add(...)

compiler = Compiler.default()
sequence, _ = compiler.compile(circuit, platform, {0: qubit})
and the `seq

Yes, it should work. At the end, to do something more general, and since execute_circuit is calling the transpiler in any case, I decided to build a dummy one where the router is returning the map {0: qubit}.

hay-k commented 2 months ago

Ok, after going through all the discussions around this topic in at least 5 different places, I want to summarize my thoughts here.

This PR touches a part of qibolab that almost everybody (including myself) agrees is wrong - qibolab does transpilation, but it shouldn't.
So, there are two separate problems: 1. the problem that this PR is trying to solve, 2. the problem that the current design of SW puts transpilation inside qibolab.
As long as 2. is not resolved, any improvements around transpilation (like this PR) should be done where the transpilation is, and they should be accepted.
\2. should not be fixed as part of this PR. If this PR is not urgent, then it should be rejected, 2. should be fixed, and then 1. fixed in a better way (if not automatically fixed by fixing 2.)
I can see now we have another parallel PR that fixes 2., and puts this PR in a weird situation without explicit discussion/agreement.
Once 2. is fixed, qibolab backend no longer does transpilation, but it still accepts qibo circuits as input, so it should know how to deal with them without causing much trouble/inconvenience for the user.
- Always requiring qibo to provide circuits for all platform qubits, even if only small subset of them are actually used puts the responsibility (rightfully) on qibo - "you don't know how to name your qubits arbitrarily, then you make sure to circumvent that limitation yourself".
- Along with the circuit accepting optional qubit map in qibolab backend makes the user experience nicer and more collaborative - qibo can submit entire circuits, or circuits with qubit subset along with a mapping, depending on what they consider more convenient. However, I don't know how well this fits into the abstract qibo backend interface.
- other solutions like dynamically creating platfrom subsets, or refactoring qibo so that it can name quibts arbitrarily, seem too complicated for such a simple problem. They can still be done because of other reasons, but I think this one is too small to motivate such changes

alecandido commented 2 months ago

Along with the circuit accepting optional qubit map in qibolab backend makes the user experience nicer and more collaborative - qibo can submit entire circuits, or circuits with qubit subset along with a mapping, depending on what they consider more convenient. However, I don't know how well this fits into the abstract qibo backend interface.

As I said somewhere else, the composition of two maps is still a map, so there is no need to support many. Qibo and the transpiler are already providing a mapping for the user, so, you can just use that one, no need to account for another inside Qibolab.

E.g. you could make a three-qubits circuit in Qibo, then pad with other sterile qubits, and map 0, 1, 2 to those you need on the platform. That should be enough. If you want to simulate, just simulate the original three-qubits circuit. No need for anything else.

other solutions like dynamically creating platfrom subsets, or refactoring qibo so that it can name quibts arbitrarily, seem too complicated for such a simple problem. They can still be done because of other reasons, but I think this one is too small to motivate such changes

Initially I misunderstood the problem, and I was thinking of a more complex one (like executing in parallel circuits on a subset of qubits - for this the views could have been useful, but you could also stack the circuits ahead of time, so maybe they are not even needed in those cases).

Edoardo-Pedicillo commented 2 months ago

This PR is not needed anymore, since the merge of qiboteam/qibocal#820

qiboteam / qibolab