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.69%. Comparing base (15fbaf9) to head (a26c06a).
Report is 134 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1534   +/-   ##
=======================================
  Coverage   99.68%   99.69%           
=======================================
  Files          80       80           
  Lines       11606    11613    +7     
=======================================
+ Hits        11570    11577    +7     
  Misses         36       36           

Flag	Coverage Δ
unittests	99.69% <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.

[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.

[MatteoRobbiati]

Firstly, thanks @mho291 for the work! It sounds in general good to me.
About input parameters, I also think as @BrunoLiegiBastonLiegi a good solution for now would be to pass parametrised gates as tuples and non-parametrised gates as single elements in the gate_set list. Another possibility would be to re-discuss how a gate is defined, and if we really need it to depend on a specific qubit number.
Anyway, this would be a way bigger discussion and I don't think this PR is the right place where to do it.

I left a couple of comments more and I would reccomend addressing the last comments from @BrunoLiegiBastonLiegi.

Thanks again :)

[mho291]

Firstly, thanks @mho291 for the work! It sounds in general good to me. About input parameters, I also think as @BrunoLiegiBastonLiegi a good solution for now would be to pass parametrised gates as tuples and non-parametrised gates as single elements in the gate_set list. Another possibility would be to re-discuss how a gate is defined, and if we really need it to depend on a specific qubit number. Anyway, this would be a way bigger discussion and I don't think this PR is the right place where to do it.

I left a couple of comments more and I would reccomend addressing the last comments from @BrunoLiegiBastonLiegi.

Thanks again :)

Thanks @MatteoRobbiati and @BrunoLiegiBastonLiegi . Yes, I have coded it to go with @BrunoLiegiBastonLiegi 's suggestion: If it's a parameterised gate, put it in a tuple. Then inside GST, check if it's a tuple, then access the parameters.

I've done this:

for gate in gate_set:
    if gate is not None:
        if isinstance(gate, tuple):
            init_args = signature(gate[0]).parameters
            params = gate[1]
            gate_class = gate[0]
        else:
            init_args = signature(gate).parameters
            params = []
            gate_class = gate
            
        angle_names = [name for name in init_args if name in {"theta", "phi", "lam"}]
        angle_values = {}
        for name, value in zip(angle_names, params):
            angle_values[name] = value

        if "q" in init_args:
            nqubits = 1
        elif "q0" in init_args and "q1" in init_args and "q2" not in init_args:
            nqubits = 2
        else:
            raise_error(
                RuntimeError,
                f"Gate {gate} is not supported for `GST`, only 1- and 2-qubits gates are supported.",
            )
        gate = gate_class(*range(nqubits), **angle_values)

It worked locally but the Pytests seemed to fail with error message
target_gates = [<qibo.gates.gates.SX object at 0x17748bc50>, <qibo.gates.gates.RX object at 0x17748bd10>, <qibo.gates.gates.PRX object at 0x17748be50>, <qibo.gates.gates.CY object at 0x17748bf50>]
and traced back to
/qibo/tomography/gate_set_tomography.py:281: in GST init_args = signature(gate).parameters
where it was complaining about the tuple input.
E TypeError: (<class 'qibo.gates.gates.RX'>, [0.7853981633974483]) is not a callable object

These TypeErrors occur for numpy and qibojit-numba:

=========================================================================================== short test summary info ===========================================================================================
FAILED test_tomography_gate_set_tomography.py::test_GST[numpy-False-target_gates0] - TypeError: (<class 'qibo.gates.gates.RX'>, [0.7853981633974483]) is not a callable object
FAILED test_tomography_gate_set_tomography.py::test_GST[numpy-True-target_gates0] - TypeError: (<class 'qibo.gates.gates.RX'>, [0.7853981633974483]) is not a callable object
FAILED test_tomography_gate_set_tomography.py::test_GST[qibojit-numba-False-target_gates0] - TypeError: (<class 'qibo.gates.gates.RX'>, [0.7853981633974483]) is not a callable object
FAILED test_tomography_gate_set_tomography.py::test_GST[qibojit-numba-True-target_gates0] - TypeError: (<class 'qibo.gates.gates.RX'>, [0.7853981633974483]) is not a callable object

Later I checked with this code which also seems to fail due to the TypeError. I've confirmed that both the pasted code and the one in the link function the same.

Oddly, the tests here are passing. Should I be concerned that tests on my local machine are failing?
I've been staring at this TypeError for a few hours now but can't find a solution. I'd really appreciate some feedback.

Thank you!

[BrunoLiegiBastonLiegi]

Not sure, can you post the complete traceback of your local error? Silly question, you have installed this branch and not the qibo main, right?

[mho291]

Silly question, you have installed this branch and not the qibo main, right?

Not silly at all! It my oversight that I did pip install qibo in a fresh environment instead of the branch. I just installed the branch and there are no errors. Thank you for guiding, I will write it down in my notes and not repeat this again. :)

[BrunoLiegiBastonLiegi]

Thanks @mho291, looks good, just some minor final suggestions.

[MatteoRobbiati]

Thanks! I would ask you to implement two more minor suggestions, but LGTM.