Updating GST to accept parameterised gates

[mho291]

Checklist:

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.68%. Comparing base (15fbaf9) to head (b05337e).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1534   +/-   ##
=======================================
  Coverage   99.68%   99.68%           
=======================================
  Files          80       80           
  Lines       11606    11612    +6     
=======================================
+ Hits        11570    11576    +6     
  Misses         36       36           

Flag	Coverage Δ
unittests	99.68% <100.00%> (+<0.01%)	⬆️

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.

[BrunoLiegiBastonLiegi]

I am no expert of tomography, but exactly what do you mean by "tomography of a parametrized gate"? I mean, I presume you are thinking of: fixing the set of parameters and performing the tomography for that specific parameter choice. So it's not really a parametrized gate anymore.

[alecandido]

@BrunoLiegiBastonLiegi the problem I believe @mho291 is trying to address should not be theoretical, but rather technical. Cf. #1502

So, you're right that once the parameters are fixed, the gate is completely analogous to any other fixed one. But from the point of view of Qibo is still holding some parameters in an attribute. Thus, it's parameterized in that sense...

[BrunoLiegiBastonLiegi]

Yeah, that I understand. This is a limitation imposed by the design choice (which was mine) of having only the class reference of gates passed rather than the actual instance. Hence:

gate_set = [gates.X, gates.Y, gates.Z]

rather than:

gate_set = [gates.X(0), gates.Y(0), gates.Z(0)]

The rationale behind this choice is that I honestly find misleading making the user pass a gate built on a qubit that is completely ignored by the GST in practice.

Therefore, I know that this choice for instance limits the applicability of GST to parametrized gates, but is that really limiting some relevant set up? (This I am actually asking because I am not sure)
What I mean is: in which cases you would like to perform GST on an RX(theta) with theta not a trivial angle that reduces to one of the standard gates X, SX, SXDG?

In any case, apart from the philosophical motivation, I would rather suggest to have a separate gate_parameters argument storing the parameters of the gates, if any. For instance:

GST(gate_set=[gates.RX, gates.Y, gates.U3],  ..., gate_parameters=[rx_param, u3_params], ...)

Or, alternatively, to pass the parametrized gates as a tuple:

GST(gate_set=[(gates.RX, rx_param), gates.Y, (gates.U3, u3_params)], ...)

[mho291]

Thanks @BrunoLiegiBastonLiegi and @alecandido. I guess the most straightforward reason to passing in a gate with parameters is so that the user does not need to transpile it to the standard gates. Will it be odd if we passed the entire gate but not use the qubit parameter, for e.g. gates.RX(qubit=15, theta=np.pi/4)? Seems less complicated than having the gate + gate parameters. But otherwise, I like the suggestion to use separate gate_parameters and we can think about it.

Following the GST wishlist here, we might want to consider adding a feature that allows the user to specify which qubit to use and gates to use for state preparation & measurements, so that GST can be used on a device that supports verbatim compilation, like the Amazon Braket devices. It's more complicated than adding gate_parameters, but I feel that adding gate_parameters now is one step towards full customisability of GST.

I've been doing GST with verbatim compilation on IQM Garnet recently so I have some gates that we can use for state preparation and measurements. There, I extracted the qubit information and used it for state preparation and measurements. But maybe let's leave that for a later task?

[alecandido]

Will it be odd if we passed the entire gate but not use the qubit parameter, for e.g. gates.RX(qubit=15, theta=np.pi/4)? Seems less complicated than having the gate + gate parameters. But otherwise, I like the suggestion to use separate gate_parameters and we can think about it.

In case, I would explore the option of using some clearly invalid value as a placeholder. More in details, not to check for this value (and just ignore it, as you would do anyhow), but rather to systematically use that to document the GST usage.

The best one would be None, i.e. gates.RX(None, theta=np.pi/4). If the type is checked anywhere, we could try with -1.

This is just to make it clear that this value is completely irrelevant (which makes explicit that the interface would be better without, but hiding it is just worse).

Following the GST wishlist here, we might want to consider adding a feature that allows the user to specify which qubit to use and gates to use for state preparation & measurements, so that GST can be used on a device that supports verbatim compilation, like the Amazon Braket devices. It's more complicated than adding gate_parameters, but I feel that adding gate_parameters now is one step towards full customisability of GST.

I've been doing GST with verbatim compilation on IQM Garnet recently so I have some gates that we can use for state preparation and measurements. There, I extracted the qubit information and used it for state preparation and measurements. But maybe let's leave that for a later task?

Definitely, let's keep for a separate task.

The one in this PR is a patch, to allow working with something that before was not accepted (unexpectedly, perhaps).
If you really want to add the qubit specification as a feature, better to plan it and designing it properly. So, a dedicated PR would be certainly recommended.

[mho291]

Thanks @alecandido . To carry on the discussion further, what might be the best way forward? I see two possible routes we can take:

1. To modify this PR to add gate parameters, qubit specification & gates for state prep and measurement.
2. To add only the gate parameters. (I feel that this will be sufficient for the general end-user like myself)

Thanks a lot! Enjoy the weekends!

[alecandido]

@mho291 in case of doubt, do less (per PR). So, 2. is definitely better than 1.. @BrunoLiegiBastonLiegi's proposal was to do even less, not more (i.e. to avoid instances, in order to avoid the qubit specification).

Concerning the alternative of doing more or less, the answer is less - and then move to further PRs.
Regarding @BrunoLiegiBastonLiegi suggestion, that's a relevant UI design question. But since it's about a UI which has a limited number of users anyhow (at least for the time being), I'd suggest you to limit the design time as well, and find a solution which is suitable to you. Whatever we do, it will never be perfect at first shot, and we'll refactor later on ^^

Thanks a lot! Enjoy the weekends!

Sorry for the late reply. I actually enjoyed it :P

[mho291]

Thanks @alecandido , let me do it like how @BrunoLiegiBastonLiegi suggested, to incorporate the gate parameters:
GST(gate_set=[(gates.RX, rx_param), gates.Y, (gates.U3, u3_params)], ...)
We can think about adding qubit specification later on.

Glad you enjoyed the weekends! ;)

[BrunoLiegiBastonLiegi]

In general I still struggle to find a strong motivation for this, as it would make more sense to me to decompose your parametrized (fixed) gate in actual non parametrized ones, which will compose your gate set for GST then.
In any case, even though the (gate, params) approach is probably not the cleanest, it should be fine for the moment.

[BrunoLiegiBastonLiegi]

Suggested change

target_gates = [gates.RX(0, np.pi/3), gates.Z(0), gates.PRX(0, np.pi/2, np.pi/3), gates.GPI(0, np.pi/7), gates.CNOT(0,1)]

not sure whether target_gates is used somewhere else, but it's not needed to run GST in principle

[BrunoLiegiBastonLiegi]

Suggested change

	gate_set (tuple or set or list): set of :class:`qibo.gates.Gate` and parameters to run
	GST on.
	E.g. target_gates = [gates.RX(0, np.pi/3), gates.Z(0), gates.PRX(0, np.pi/2, np.pi/3),
	gates.GPI(0, np.pi/7), gates.CNOT(0,1)]
	gate_set = [(g.__class__, [g.parameters[i] for i in range(len(g.parameters))])
	if g.parameters else (g.__class__, []) for g in target_gates]
	gate_set (tuple or set or list): set of :class:`qibo.gates.Gate` to run
	GST on. In case a gate is parametrized, it has to be passed as a tuple ``(gate, params)``
	E.g. ``gate_set = [(gates.RX, np.pi/3), gates.Z, (gates.PRX, [np.pi/2, np.pi/3])]``

[BrunoLiegiBastonLiegi]

Suggested change

	init_args = signature(gate[0]).parameters
	params = gate[1]
	angle_names = [name for name in init_args if name in {"theta", "phi"}]
	angle_values = {}
	for name, value in zip(angle_names, params): # Zip ensures correct order
	angle_values[name] = value
	params = {}
	if isinstance(gate, tuple):
	params.update({
	angle: param
	for angle, param in zip(["theta", "phi"][:len(gate[1])], gate[1])
	})
	gate = gate[0]

if we clearly document how you are supposed to pass the parameters, it is safe to assume that each input tuple is identifying a parametrized gate

[mho291]

Thanks Andrea! I realise that if we do

for angle, param in zip(["theta", "phi"][:len(gate[1])], gate[1])

it won't work for gates like gates.GPI(0, np.pi) because GPI gate doesn't have theta parameter. I run into this error message:

---> 46 gate = gate(*range(nqubits), **params)

TypeError: GPI.__init__() got an unexpected keyword argument 'theta'

Hence my train of thought is that if we search for theta or phi or both, then we just extract that angle_name out and assign the parameter directly (Lines 295 to 302)

angle_names = [name for name in init_args if name in {"theta", "phi"}]

angle_values = {}
for name, value in zip(angle_names, params):  # Zip ensures correct order
    angle_values[name] = value

[BrunoLiegiBastonLiegi]

with the change above

Suggested change

	gate = gate[0](*range(nqubits), **angle_values)
	gate = gate(*range(nqubits), **params)

[BrunoLiegiBastonLiegi]

Suggested change

	gate_set = [
	(
	(g.__class__, [g.parameters[i] for i in range(len(g.parameters))])
	if g.parameters
	else (g.__class__, [])
	)
	for g in target_gates
	]
	gate_set = [
	(
	(g.__class__, g.parameters)
	if g.parameters
	else g.__class__
	)
	for g in target_gates
	]

isn't this working?

[BrunoLiegiBastonLiegi]

same as above

[mho291]

Thanks @BrunoLiegiBastonLiegi for the comments.

In general I still struggle to find a strong motivation for this, as it would make more sense to me to decompose your parametrized (fixed) gate in actual non parametrized ones, which will compose your gate set for GST then.

True, I do see your point of view. However, if I wanted to quickly do GST for parameterised gates without the decomposition, then we'll probably need this small functionality. Correct me if I'm wrong: I have the impression that since qw11q device uses RX gate as its native gate, then we'll need to pass in a parameter regardless, right?

Anyway, back to the PR, when I was constructing the gate_set, I thought that maybe it'd be better to standardise the input. Instead of
gate_set = [gates.X, (gates.RX, [np.pi/3]), gates.Z, (gates.PRX, [np.pi/2, np.pi/3]), (gates.GPI, [np.pi/7])],
we can leave a blank [] for gates that don't have any parameters. So the gate_set will look like this:
gate_set = [(gates.X, []), (gates.RX, [np.pi/3]), (gates.Z, []), (gates.PRX, [np.pi/2, np.pi/3]), (gates.GPI, [np.pi/7])]

From an end-user perspective, the most convenient way is to dump the entire gate and its parameters, including qubit specifications, into the gate_set. However, since we're doing it incrementally, I find it easier to configure the gate_set when all gates are within the parentheses (gates.X, []) rather than needing to remember which gates to avoid wrapping with parentheses. I'd like to hear your opinion whether my thought process makes sense!

[BrunoLiegiBastonLiegi]

True, I do see your point of view. However, if I wanted to quickly do GST for parameterised gates without the decomposition, then we'll probably need this small functionality.

Yeah, fair enough, it's surely faster to just allow for passing the parametrized gate here, rather than manually decomposing

Correct me if I'm wrong: I have the impression that since qw11q device uses RX gate as its native gate, then we'll need to pass in a parameter regardless, right?

Yes and now, because if I remember correctly our RX is implemented as GPI2(pi/2) pulses. In any case, the decomposition happens internally anyway when you execute on hardware, the transpiler argument is indeed there for that.

Anyway, back to the PR, when I was constructing the gate_set, I thought that maybe it'd be better to standardise the input. Instead of gate_set = [gates.X, (gates.RX, [np.pi/3]), gates.Z, (gates.PRX, [np.pi/2, np.pi/3]), (gates.GPI, [np.pi/7])], we can leave a blank [] for gates that don't have any parameters. So the gate_set will look like this: gate_set = [(gates.X, []), (gates.RX, [np.pi/3]), (gates.Z, []), (gates.PRX, [np.pi/2, np.pi/3]), (gates.GPI, [np.pi/7])]
From an end-user perspective, the most convenient way is to dump the entire gate and its parameters, including qubit specifications, into the gate_set. However, since we're doing it incrementally, I find it easier to configure the gate_set when all gates are within the parentheses (gates.X, []) rather than needing to remember which gates to avoid wrapping with parentheses. I'd like to hear your opinion whether my thought process makes sense!

I understand your wish of standardizing the inputs, but personally I am not a fan of making the user specify a non parametrized gate as (gate, []) everytime. Say for instance, that I want to run GST on X, Y and Z then I have to call

GST(gate_set=[(X, []), (Y, []), (Z, [])])

not the best in my opinion. We could actually leverage the asimettry, instead, and infer from the presence of a tuple the presence of a parametrized gate.