Use of wire_names

[csookim]

This covers issue #1073.

Updates

• circuit.wire_names is copied to Platform.wire_names each time execute_circuit(circuit, ...) is run.
• In get_qubit(qubit), when qubit is an integer, it returns the corresponding physical qubit name from wire_names.
• In some rules, get_qubit() is used to retrieve QubitId.
• Test files is updated.

Checklist:

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

[csookim]

Initially, I refactored the code so that all rules() return QubitId and all create_[]_pulse() functions receive QubitId as a parameter. But since there are many usages of these functions in hardware backends, I reverted the changes to avoid making a breaking change.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.88%. Comparing base (a30c552) to head (134efd2).
Report is 13 commits behind head on 0.1.

Additional details and impacted files

@@            Coverage Diff             @@
##              0.1    #1079      +/-   ##
==========================================
- Coverage   69.89%   69.88%   -0.02%     
==========================================
  Files          64       64              
  Lines        6876     6880       +4     
==========================================
+ Hits         4806     4808       +2     
- Misses       2070     2072       +2     

Flag	Coverage Δ
unittests	69.88% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[alecandido]

Other than the handling of the temporary fix, it seems fine to me :)

[alecandido]

Do you actually need this fix? Or can we rely just on qiboteam/qibo#1500?

[csookim]

Do you actually need this fix?

No, if qiboteam/qibo#1500 is applied, we can remove the temporary fix.

Currently, the circuit implementation in qibo uses q{i} as the default qubit names and does not support setting wire_names as list[int] through the setter. And there are minor issues where wire_names are not copied when copying the circuit. I included a temporary fix to avoid adding meaningless lines in the test files.

[alecandido]

We have two options:

1. keep the workaround and merge it (with temp fix lines)
2. start targeting the branch in Transpiler refactor qibo#1500 and block the merge of this PR to its release (no temp fix lines)

@stavros11?

[alecandido]

So, while before I was more undecided about what to do, cf. #1079 (comment), I now believe we should not block this PR behind qiboteam/qibo#1500. The temporary workaround may work well enough, such that we could move forward just relying on it, and get back whenever the other one will be merged (that may even be not required at all for 0.1, as it is approaching EOL, and there may be no further releases).

However, I'd still wait for @stavros11 opinion as well.

(btw, in the meanwhile @csookim you may merge/rebase, since #1076 got merged)

[stavros11]

I was going to say that keeping on hold probably takes less effort since if we merge we will then have to also open another PR to undo the temporary fix. However, in order to include this in the next release we can probably merge as it is. I would still open an issue to remove the temporary fix, though.

The only issue is that if the temporary fix does not work well with qibo after qiboteam/qibo#1500 is merged, then we may need to release qibolab 0.1 again, but I don't think that's the case.

Other than that, thanks @csookim for the updates and the compiler refactoring, it looks good to me now.

[alecandido]

The only issue is that if the temporary fix does not work well with qibo after qiboteam/qibo#1500 is merged, then we may need to release qibolab 0.1 again, but I don't think that's the case.

Ok, wait a second: this is done here because you expect that all the circuits coming out of the transpiler should be padded with all the qubits present in the platform, eventually. Correct? @csookim

If that's the case, the moment this will be done by the transpiler, the temporary fix should be just ineffective.
If false, we should make such that it will happen like that (i.e. provide what we expect as the transpiler output after qiboteam/qibo#1500, only if it's not already the case).

[stavros11]

If that's the case, the moment this will be done by the transpiler, the temporary fix should be just ineffective.

I think so, because circuit._wire_names does not exist after qiboteam/qibo#1500 (should be replaced by circuit.wire_names), but I have not reviewed this thouroughly, so it is better if @csookim confirms.

EDIT: Ignore this comment, I am actually wrong.

[csookim]

Ok, wait a second: this is done here because you expect that all the circuits coming out of the transpiler should be padded with all the qubits present in the platform, eventually. Correct? @csookim

Yes, it should be padded with the same name as the connectivity graph nodes (= list(platform.qubits)), but this isn’t happening currently.

If that's the case, the moment this will be done by the transpiler, the temporary fix should be just ineffective.

If the transpiler is updated, we must remove the temp fix cause it overwrites the order circuit.wire_names.

[alecandido]

If the transpiler is updated, we must remove the temp fix cause it overwrites the order circuit.wire_names.

But, if the transpiler is padding on its side, you should not be entering the conditional, isn't it?

qibolab/src/qibolab/backends.py

Lines 128 to 131 in 9e9d625

	# This should be done in qibo side
	# Temporary fix: overwrite the wire names
	if not all(q in circuit.wire_names for q in self.qubits):
	circuit._wire_names = self.qubits

Discussion still ongoing

[csookim]

But, if the transpiler is padding on its side, you should not be entering the conditional, isn't it?

Oh, sorry. If the transpiler starts padding consistently with the connectivity, then the temporary fix becomes ineffective. It only has an effect when the transpiler doesn’t consider the connectivity.

[stavros11]

@alecandido based on the last discussion, I think this can be merged and released with qibolab 0.1.10. The temporary fix should be ineffective after qiboteam/qibo#1500 is merged, therefore we will not have to release another 0.1 to remove it after we release the new qibo.

[alecandido]

Just to fully understand, and check we are on the same page.

Padding consistently is not required. If you send a Circuit the QibolabBackend will not validate it for the presence of all qubits, and now that the .wire_names are there, you just pass the circuit down to the compiler, that will execute gates based on the qubits they're applied on, translated with the wires.

So, given this situation, padding is fully optional (unless I'm missing something).

We can then resolve the present situation in two alternative ways:

1. the current one, in which we condition the fix on the padding, and we require the refactored transpiler in Transpiler refactor qibo#1500 to always pad
2. we give up on padding, since not necessary, but we need to switch the conditional to identify the pre- Transpiler refactor qibo#1500 outputs, in which Circuit.wire_names is not passed
   • this identification can be done by inverting the check: instead of checking what "it should be", we exactly check what "it should not" - i.e. the pre- Transpiler refactor qibo#1500 of having as many qubits as c.nqubits, and c.wire_names resolving to ["q0", ..., "q{n-1}"]

I believe 2. to be slightly more elegant, but to be fair: it's just a temporary fix, I'm not really concerned.
Eventually, we'll stop using Qibolab 0.1, and in Qibolab 0.2 at some point we'll drop the temporary fix, and drop support for older Qibo versions.

To me, both solutions are fine, including the current one, provided that qiboteam/qibo#1500 will properly implement its side of the contract.
If we all agree on this, we can keep going and merge as it is.

[csookim]

Or we could check like this:

Before

 if not all(q in circuit.wire_names for q in self.qubits): 
     circuit._wire_names = self.qubits 

After

 if not all(q in self.qubits for q in circuit.wire_names): 
     circuit._wire_names = self.qubits[:circuit.nqubits]

To give up on padding and consider the names only.

[alecandido]

To give up on padding and consider the names.

Yes, that's definitely an option.

I had in mind to be as specific as:

 if circuit.wire_names == [f"q{n}" for n in range(circuit.nqubits)]: 
     circuit._wire_names = self.qubits 

because this should make extremely likely for the conditional to expire after qiboteam/qibo#1500.

However, if the condition you're proposing is essentially what is needed to execute, it's even cleaner to use that one instead (and it should also expire after qiboteam/qibo#1500, since the transpiler will make sure to satisfy that).

[csookim]

 if circuit.wire_names == [f"q{n}" for n in range(circuit.nqubits)]: 
     circuit._wire_names = self.qubits 

In this case, there could be an issue if the qubit names are q{i}. Unlikely but for safety, I would prefer using all(q in self.qubits for q in circuit.wire_names).