Adding dimension property to comparison of XPowGate and ZPowGate #6005
Conversation
|
will get to the code style stuff soon, but it only occurred to me just now to ask: should the dimension be added only when it is not 2? changing a lot of json test files... |
There was a problem hiding this comment.
Thanks for opening the PR Joe! I agree that we should include dimension in the json serialization only when dimension != 2.
As next steps, please do the following:
- Update the
json_dictmethod to includedimensiononly when it's != 2. You can do this via
d = protocols.obj_to_dict_helper(self, ['exponent', 'global_shift'])
if self.dimension != 2:
d['dimension'] = self.dimension
return d- Revert the changes to all the json test data files.
- Add a new json test data for XPowGate(dimension=3) and ZPowGate(dimension=3) and test that the roundtrip works. Instead of adding
test_json(), you should directly update thecirq-core/cirq/protocols/json_test_data/XPowGate.jsonandcirq-core/cirq/protocols/json_test_data/XPowGate.reprfiles with adding the output fromcirq.to_json(x3)andrepr(x3).
|
@tanujkhattar Hi! I made the updates and added the test file you suggested. I am not sure if I completely finished the tests correctly though. I need more explanation on this part:
|
cirq-core/cirq/ops/common_gates.py
Outdated
| def _value_equality_values_(self): | ||
| return self._canonical_exponent, self._global_shift, self._dimension |
There was a problem hiding this comment.
I think it's better to call the _value_equality_values_ of parent class and append it the tuple with self.dimension. Also, we need to update approx equality as well (and add a test for the same). So,
| def _value_equality_values_(self): | |
| return self._canonical_exponent, self._global_shift, self._dimension | |
| def _value_equality_values_(self): | |
| return (*super()._value_equality_values_(), self._dimension) | |
| def _value_equality_approximate_values_(self): | |
| return (*super()._value_equality_approximate_values_(), self._dimension) |
Ditto for ZPowGate.
There was a problem hiding this comment.
Should be pushed now. I also pushed up removing test_json() because I forgot to remove it in the previous round
…lue_equality_approximate_values_ For X and Z PowGates
…ntumlib#6005) * adding methods so that dimension is included in _value_equality_values_ and _json_dict_ * quick fix to use canonical exponent in added methods * needed to update the json for unit tests as well * fix code style * updating last jsons to include dimension * Revert "updating last jsons to include dimension" This reverts commit 7387710. * Revert "needed to update the json for unit tests as well" This reverts commit 473c99e. * adding dimension to json dict only if dimension is not the default * adding json and repr files to test XPowGate and ZPowGate dimension * updating json and repr files to test XPowGate and ZPowGate dimension and removing individual files * updating _value_equality_values_ function after review and adding _value_equality_approximate_values_ For X and Z PowGates * removing redundant test for read_json --------- Co-authored-by: Tanuj Khattar <tanujkhattar@google.com>
Attempting to solve issue #5995