Update gate.clifford value in rotations given a new angle

[MatteoRobbiati]

This PR fixes the gate.clifford value when a new theta is set into a rotational (or controlled-rotational) gate.

I am not sure this approach is the best, in particular considering the already implemented @Gate.parameters.setter in the abstract.py file. Do you have any feedback on this @stavros11 (I bother you because you implemented the ParametrizedGate some years ago)?

Checklist:

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

[renatomello]

This PR fixes the gate.clifford value when a new theta is set into a rotational (or controlled-rotational) gate.

I am not sure this approach is the best, in particular considering the already implemented @Gate.parameters.setter in the abstract.py file. Do you have any feedback on this @stavros11 (I bother you because you implemented the ParametrizedGate some years ago)?

I think it would be cleaner to transform self.clifford into a @property and create a setter for it that calculates the value on the fly.

[alecandido]

I think it would be cleaner to transform self.clifford into a @property and create a setter for it that calculates the value on the fly.

Once it is a property, you shouldn't even need a setter. The property getter (the function decorated with @property) can already determine whether it is Clifford when the request is made, based on current parameters.

Still some tests to be fixed

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (44f5b83) to head (704cf98).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1257   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          73       73           
  Lines       10638    10683   +45     
=======================================
+ Hits        10623    10668   +45     
  Misses         15       15           

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

[stavros11]

Do you have any feedback on this @stavros11 (I bother you because you implemented the ParametrizedGate some years ago)?

Thanks @MatteoRobbiati. The current approach looks good to me, I believe there is only still some issue with the tests (at least the CI).

In principle you don't even need the self._clifford attribute, you can define the property as

@property
def clifford(self):
    return False

and overwrite it to return True for Clifford gates and rotations (depending on angle). This may end up being more lines of code, though, since overwriting the property is more than overwriting the attribute.

[MatteoRobbiati]

and overwrite it to return True for Clifford gates and rotations (depending on angle). This may end up being more lines of code, though, since overwriting the property is more than overwriting the attribute.

Exactly! I decided to use self._clifford because actually I wanted self.clifford to be a property in terms of usability but still I needed to manipulate the value on the fly when updating the parameters (and I thought it is fine to modify the self._clifford covertly). In the same time I didn't know if defining it as property in all gates was a good option. Is this what you suggest?

I am open to modifications in case :)

[stavros11]

Exactly! I decided to use self._clifford because actually I wanted self.clifford to be a property in terms of usability but still I needed to manipulate the value on the fly when updating the parameters (and I thought it is fine to modify the self._clifford covertly). In the same time I didn't know if defining it as property in all gates was a good option. Is this what you suggest?

Only in Clifford gates (in cases where it should return True). For False it will just inherit the abstract. But in any case, the self._clifford solution is fine for me too.

[alecandido]

Only in Clifford gates (in cases where it should return True). For False it will just inherit the abstract. But in any case, the self._clifford solution is fine for me too.

I proposed this to @MatteoRobbiati to limit the amount of changes, but even the property is certainly an option, and a bit cleaner (since you don't need a mutable variable per instance, but if the information is stored once per class is enough).

So, if you're willing to pay the price of 2-3 extra lines per Clifford class, you'd save one attribute for every instance of a Gate (making them marginally lighter).

[renatomello]

Only in Clifford gates (in cases where it should return True). For False it will just inherit the abstract. But in any case, the self._clifford solution is fine for me too.

I proposed this to @MatteoRobbiati to limit the amount of changes, but even the property is certainly an option, and a bit cleaner (since you don't need a mutable variable per instance, but if the information is stored once per class is enough).

So, if you're willing to pay the price of 2-3 extra lines per Clifford class, you'd save one attribute for every instance of a Gate (making them marginally lighter).

I second this. It seems like a better solution to me.

[MatteoRobbiati]

Thanks for the suggestions! I left self._clifford only in the general Unitary gate, where it is used in the clifford.setter.

[stavros11]

Thanks @MatteoRobbiati, looks good to me.