Native gates directly defined by the platform

[Edoardo-Pedicillo]

This PR fixes #1075. Now the dummy_transpiler takes the native gates directly from the platform (assuming all qubits and pairs to have the same ones) and also the compiler is updated taking into account the new rules. This PR doesn't solve the problem of define any possible native gates set, indeed Qibo requires them to be between the ones defined here https://github.com/qiboteam/qibo/blob/312efb9d6934e85cfdaafcdb7027cdfe46bf87c4/src/qibo/transpiler/unroller.py#L51-L59

Otherwise it will raise an error
https://github.com/qiboteam/qibo/blob/312efb9d6934e85cfdaafcdb7027cdfe46bf87c4/src/qibo/transpiler/unroller.py#L82-L83

[codecov]

Codecov Report

Attention: Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.

Project coverage is 97.22%. Comparing base (59f95e3) to head (a04745a).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
src/qibocal/auto/transpile.py	86.84%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1077      +/-   ##
==========================================
- Coverage   97.29%   97.22%   -0.08%     
==========================================
  Files         123      123              
  Lines        9845     9880      +35     
==========================================
+ Hits         9579     9606      +27     
- Misses        266      274       +8     

Flag	Coverage Δ
unittests	97.22% <86.84%> (-0.08%)	⬇️

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

Files with missing lines	Coverage Δ
src/qibocal/auto/transpile.py	88.23% <86.84%> (-2.68%)	⬇️

... and 2 files with indirect coverage changes

[alecandido]

Mostly style comments, but those related to side effects and neglecting the existing platform.natives attributes are substantial

[alecandido]

I wonder if Z should be somehow directly inferred from RZ by the transpiler itself...

(implying that is not platform's/Qibocal's business)

[Edoardo-Pedicillo]

In theory yes, I am using Qibolab compiler and NativeGates convention to differentiate between the two.

[alecandido]

Fine, but is there a reason to differentiate?

I.e. with the current compiler, the RZ will always be implemented as virtual phase, and that will be the same for the Z as well...

[Edoardo-Pedicillo]

Fine, but is there a reason to differentiate?

No, maybe @andrea-pasquale could have an explanation.

[alecandido]

Is there a reason to start from parameters, instead of starting directly from the natives listing exposed by the platform?
https://github.com/qiboteam/qibolab/blob/bfb48cc2d9ec5e563110720889dc74acca6e604c/src/qibolab/backends.py#L64-L79

[Edoardo-Pedicillo]

I am not sure I understood, this function should return the native gates that have a rule in the default compiler, so it will not work if the iSWAP is in the platform (this was my first attempt but as explained before the result was not what I was looking for, I am not sure this is intended).

[alecandido]

Wait a second: this function is used just to feed set_compiler, in which you're doing the following

qibocal/src/qibocal/auto/transpile.py

Lines 151 to 154 in e1c0047

	if gate not in compiler.rules:
	rules[gate] = create_rule(native)
	else:
	rules[gate] = compiler.rules[gate]

To me, that means that you're trying to support rules beyond the default compiler. Including iSWAP, if available.

I'm sure I misunderstood something in your reply...

[alecandido]

Using closures is always delicate, so, I'd suggest moving it in a function on its own, at least will make it clear which are the captured variables

def create_rule(native):
    def rule(qubits_ids, platform, parameters=None):
        if len(qubits_ids[1]) == 1:
            native_gate = platform.qubits[tuple(qubits_ids[1])].native_gates
        else:
            native_gate = platform.pairs[tuple(qubits_ids[1])].native_gates
        pulses = getattr(native_gate, native).pulses
        return PulseSequence(pulses), {}

    return rule

[alecandido]

P.S.: type hints would also help

[alecandido]

Also, usually it is better to avoid mutating objects, especially nested ones.
I could lift the suggestion by one level, and suggest creating the entire backend (here just returning the compiler, instead of modifying the backend). But let's limit to the compiler itself.

You could create an empty dictionary rules, and here do

Suggested change

	backend.compiler[gate] = rule
	rules[gate] = rule

eventually setting the compiler out of the loops as:

    backend.compiler = Compiler(rules=rules)

(in principle, you can avoid even modifying the dictionary rules, by defining it immediately through comprehension - but I get this is a bit unfamiliar).

[alecandido]

Properly document that this function is not pure, since it has the side effect of modifying the input backend (in particular, affecting its .compiler attribute).

I know this seems pedantic, but losing track of this changes may have effects later on, when they will become unexpected.

[Edoardo-Pedicillo]

My idea at the beginning was to make the set_compiler function in each protocol that executes circuits explicit. I decided to put it here to make my life easier but I will open an issue about it.

[alecandido]

My idea at the beginning was to make the set_compiler function in each protocol that executes circuits explicit.

No need.

I agree with your choice, since there is nothing protocol-specific, so it should not be repeated over and over.

to make my life easier

Everyone's life easier. Good choice :)

I decided to put it here

Even better, you could have put it here

qibocal/src/qibocal/auto/execute.py

Lines 263 to 264 in f8f69de

	backend = construct_backend(backend="qibolab", platform=platform)
	platform = self.platform = backend.platform

if it were the single place where the backend was created.

Since unfortunately it is not, instead of this:

qibocal/src/qibocal/protocols/state_tomography.py

Lines 105 to 106 in f8f69de

	backend = construct_backend("qibolab", platform=platform)
	transpiler = dummy_transpiler(backend)

you could actually just make a single function, and invoke as

    backend, transpiler = construct_qibolab_backend(platform)

considering that this double functions invocation is repeat over and over.

https://github.com/search?q=repo%3Aqiboteam%2Fqibocal%20construct_backend&type=code

(in any case, this one could be an issue - but it's especially relevant for 0.2, since we won't maintain 0.1 for long)

[alecandido]

It seems I forgot this in the previous review: I guess this is just a leftover.