Separate channels from natives

[alecandido]

Closes #982

• restore support for symmetric two qubit gates

[alecandido]

@stavros11 I had to touch again the compiler, because the Qubit and natives of course are pretty much involved.

I now have a dedicated call for Z and RZ

qibolab/src/qibolab/compilers/compiler.py

Lines 123 to 129 in 7f8e47f

	if isinstance(gate, (gates.Z, gates.RZ)):
	qubit = platform.get_qubit(gate.target_qubits[0])
	return rule(gate, qubit)
	if len(gate.qubits) == 1:
	qubit = platform.get_qubit(gate.target_qubits[0])
	return rule(gate, natives.single_qubit[qubit.name])

this is because they are creating the sequence on the fly

qibolab/src/qibolab/compilers/default.py

Lines 16 to 23 in 7f8e47f

	def z_rule(gate: Gate, qubit: Qubit) -> PulseSequence:
	"""Z gate applied virtually."""
	return PulseSequence([(qubit.drive.name, VirtualZ(phase=math.pi))])
	def rz_rule(gate: Gate, qubit: Qubit) -> PulseSequence:
	"""RZ gate applied virtually."""
	return PulseSequence([(qubit.drive.name, VirtualZ(phase=gate.parameters[0]))])

But I wonder whether it's worth to treat them as the RX, just hard-coding the sequence pulse (it could be a (ch, null) in the JSON, and then load it only to receive the channel name).

It's true that in principle we may infer the channel, giving the qubit name and its role. But this is true in general, i.e. we can replace ChannelId = str by ChannelId = tuple[QubitId, Literal["drive", "drive12", "flux", "probe", "acquire"]]. And I would do in a separate PR in any case (there would be also the problem about how to address the cross-resonance channels, but that we can address with a further pair ("drive_cross", name)).

[alecandido]

qibolab/src/qibolab/compilers/compiler.py

Lines 63 to 98 in 7f8e47f

	def __setitem__(self, key: type[gates.Gate], rule: Callable):
	"""Set a new rule to the compiler.
	If a rule already exists for the gate, it will be overwritten.
	"""
	self.rules[key] = rule
	def __getitem__(self, item: type[gates.Gate]) -> Callable:
	"""Get an existing rule for a given gate."""
	try:
	return self.rules[item]
	except KeyError:
	raise KeyError(f"Compiler rule not available for {item}.")
	def __delitem__(self, item: type[gates.Gate]):
	"""Remove rule for the given gate."""
	try:
	del self.rules[item]
	except KeyError:
	KeyError(f"Cannot remove {item} from compiler because it does not exist.")
	def register(self, gate_cls):
	"""Decorator for registering a function as a rule in the compiler.
	Using this decorator is optional. Alternatively the user can set the rules directly
	via ``__setitem__``.
	Args:
	gate_cls: Qibo gate object that the rule will be assigned to.
	"""
	def inner(func):
	self[gate_cls] = func
	return func
	return inner

All this interface (but __getitem__) is never used and never tested. We should that (testing), and maybe even mention in the docs.

Not in this PR, but it's worth an issue (at least concerning tests).

[alecandido]

In 1c8c52e I introduced a container, alternative to the plain dict, to access the symmetric gates.

In this case, I tried to wrap a dict, instead of subclassing. However, it turned out dict has an extensive API, and just exposing a bit of it seemed much redundant.
Despite Pydantic not being incredibly friendly to these subclasses, it's much better to repeat the solution of PulseSequence. And that's what I did in 6d67b0e

[codecov]

Codecov Report

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

Project coverage is 41.59%. Comparing base (a072cd5) to head (9ac2d23).

Files	Patch %	Lines
src/qibolab/compilers/compiler.py	87.50%	3 Missing ⚠️
src/qibolab/parameters.py	95.83%	1 Missing ⚠️
src/qibolab/platform/platform.py	92.85%	1 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           serialization-interface     #983      +/-   ##
===========================================================
- Coverage                    41.63%   41.59%   -0.04%     
===========================================================
  Files                           79       79              
  Lines                         5421     5421              
===========================================================
- Hits                          2257     2255       -2     
- Misses                        3164     3166       +2     

Flag	Coverage Δ
unittests	41.59% <94.44%> (-0.04%)	⬇️

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.

[stavros11]

Thanks @alecandido. Other than the native_gates property which could be easily implemented here and I believe would improve the quality of life of qibocal developers, we can probably merge this. My other concerns are in the comments below, but are not strictly necessary to fix in this PR.

qibolab/src/qibolab/compilers/compiler.py

Lines 63 to 98 in 7f8e47f

	def __setitem__(self, key: type[gates.Gate], rule: Callable):
	"""Set a new rule to the compiler.
	If a rule already exists for the gate, it will be overwritten.
	"""
	self.rules[key] = rule
	def __getitem__(self, item: type[gates.Gate]) -> Callable:
	"""Get an existing rule for a given gate."""
	try:
	return self.rules[item]
	except KeyError:
	raise KeyError(f"Compiler rule not available for {item}.")
	def __delitem__(self, item: type[gates.Gate]):
	"""Remove rule for the given gate."""
	try:
	del self.rules[item]
	except KeyError:
	KeyError(f"Cannot remove {item} from compiler because it does not exist.")
	def register(self, gate_cls):
	"""Decorator for registering a function as a rule in the compiler.
	Using this decorator is optional. Alternatively the user can set the rules directly
	via ``__setitem__``.
	Args:
	gate_cls: Qibo gate object that the rule will be assigned to.
	"""
	def inner(func):
	self[gate_cls] = func
	return func
	return inner

All this interface (but __getitem__) is never used and never tested. We should that (testing), and maybe even mention in the docs.

Not in this PR, but it's worth an issue (at least concerning tests).

The idea was to allow the users to alter the compiler by adding more rules or modifying existing ones. However, dropping this interface is also an option. Modifications are still allowed on compiler.rules through Python's dict interface anyway, so it is not super useful to reinvent it here.

[stavros11]

I don't fully like all these ifs as it's a bit against of the idea of having the dict of rules (which can also be modified by user) and delegate as much possible to that. However, I have not spend time to thought in detail for a better mechanism so I don't want to just complain without providing any alternatives. Maybe a simple one, without thinking much, would be to pass the whole platform to each rule, as it was before.

If you also don't fully like it, maybe we could open an issue and try to improve in the future. I am not sure if it's for 0.2 as the compiler is not the usual user interfacing kind, but could also be thought as interface for another kind of users.

[alecandido]

I don't fully like all these ifs as it's a bit against of the idea of having the dict of rules (which can also be modified by user) and delegate as much possible to that. However, I have not spend time to thought in detail for a better mechanism so I don't want to just complain without providing any alternatives. Maybe a simple one, without thinking much, would be to pass the whole platform to each rule, as it was before.

I'm not fully convinced, but I also don't fully like the idea of passing everything down below.
This is truly the trade-off I arrived, in which I'm essentially dividing in 2+ categories: single-qubit, two-qubit, special handling.

Since the special is a catch-all category, maybe it's worth to just pass everything down just in this case.
For the other two categories, it should be pretty clear what to do. But unfortunately you can isolate them only if you remove the special ones before, since M and Align may also apply to one and two qubits. Maybe we could have a list of those that should be treated as special, and expose it to the user for extensions.

If you also don't fully like it, maybe we could open an issue and try to improve in the future. I am not sure if it's for 0.2 as the compiler is not the usual user interfacing kind, but could also be thought as interface for another kind of users.

In principle, the compiler is only used by the user for extensions. Since we do not believe that extensions are immediately needed, we could keep it private, and publish only at a later stage.

[stavros11]

This class looks like a lot of code overhead, that is also quite obscure for someone without experience with pydantic, just to handle the pairs of symmetric two-qubit gates, but at this stage I trust that there was no simpler solution.

[alecandido]

This class looks like a lot of code overhead, that is also quite obscure for someone without experience with pydantic, just to handle the pairs of symmetric two-qubit gates, but at this stage I trust that there was no simpler solution.

Yes, I also don't like it. And I even tried the alternative, as written above #983 (comment)

I hoped Pydantic had a better interface for subclassing builtins, but that's the easiest. Already importing pydantic_core is a bit of failure from the point of view of maintainability (it's truly their qibo_core, btw Pydantic people are those maintaining PyO3, the framework we're also using for qibo_core).

But that's truly the easiest way I found 😞

[alecandido]

This is merged into #971, since the stricter review is happening together in any case.

I'll go through the comments, implement in #971 the simple ones, and create issues for the remaining ones.

[alecandido]

The idea was to allow the users to alter the compiler by adding more rules or modifying existing ones. However, dropping this interface is also an option. Modifications are still allowed on compiler.rules through Python's dict interface anyway, so it is not super useful to reinvent it here.

I'm undecided here: on the one side is not that bad, but mostly because I don't expect people to play much with the compiler, for the time being. In which case it would be just unused (as it is).

Instead, the moment people would start using that, maybe the full dictionary API might become handy, so we should encourage accessing the attribute.

So, maybe it's really the case of dropping it. But I'd keep @register(Gate), it seems convenient.

[alecandido]

So, maybe it's really the case of dropping it. But I'd keep @register(Gate), it seems convenient.

Done in d9a210f

[alecandido]

I don't have a clear solution for the other two conversations in this PR, so I'll do nothing.

For me, they are both a bit unsatisfactory, but also not critical. Thus, I will not open issues.
If you believe we should review them before the 0.2 release, feel free to open issues and add to the milestone @stavros11