Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Quality Fixes in StatePreparationChannel #4503

Merged
merged 9 commits into from
Sep 28, 2021

Conversation

AnimeshSinha1309
Copy link
Contributor

@AnimeshSinha1309 AnimeshSinha1309 commented Sep 17, 2021

Addressing the comments made in #4482
This pull request fixes some of the code quality issues that were created in the previous pull request.

  • Format changes and not using staticmethods
  • Using cirq testing to write tests
  • Name added as a parameter and in serialization

Fixed many of the nits mentioned in quantumlib#4482, value-equality not a part of this commit.
@AnimeshSinha1309 AnimeshSinha1309 requested review from cduck, vtomole and a team as code owners September 17, 2021 07:15
@CirqBot CirqBot added the size: S 10< lines changed <50 label Sep 17, 2021
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Sep 17, 2021
@AnimeshSinha1309 AnimeshSinha1309 changed the title Addressing the comments made in #4482 Code Quality Fixes in StatePreparationChannel Sep 17, 2021
@AnimeshSinha1309
Copy link
Contributor Author

@tanujkhattar I don't know how to use ValueEquality, because then I am forced to specify an exact comparison value, which does not exist. It makes the test gate == eval(repr(gate)) fail, because the reconstruction is not exact. MatrixGate is similar, they don't use value equality. I have changed the code around testing equality a little to make it similar to that of MatrixGate.

State Preparation Channel got equality comparison similar to that of Matrix Gate.
@AnimeshSinha1309
Copy link
Contributor Author

Without getting @value_equality to work, I am also getting errors with EqualsTester.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, we shouldn't use value_equality here.

Left a few more comments for bugfixes around handling the name parameter of the class. We can merge once these are fixed.

name parameter added to constructor, JSON, and repr serialization as well as test data.
The name has been changed in the JSON and repr protocol tests to make the older version fail.
Had to run pylint.
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nit.

Please fix the nit and we can merge.

Two gates with different names are still equal, added tests for that.
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tanujkhattar tanujkhattar merged commit 44e063b into quantumlib:master Sep 28, 2021
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
* Fixing Minor Errors

Fixed many of the nits mentioned in quantumlib#4482, value-equality not a part of this commit.

* Approx Equality support added

State Preparation Channel got equality comparison similar to that of Matrix Gate.

* Added name parameter to state preparation channel

name parameter added to constructor, JSON, and repr serialization as well as test data.

* Custom names in serialization tests

The name has been changed in the JSON and repr protocol tests to make the older version fail.

* Format and Lint Fix

Had to run pylint.

* Adding tests to ignore name

Two gates with different names are still equal, added tests for that.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
* Fixing Minor Errors

Fixed many of the nits mentioned in quantumlib#4482, value-equality not a part of this commit.

* Approx Equality support added

State Preparation Channel got equality comparison similar to that of Matrix Gate.

* Added name parameter to state preparation channel

name parameter added to constructor, JSON, and repr serialization as well as test data.

* Custom names in serialization tests

The name has been changed in the JSON and repr protocol tests to make the older version fail.

* Format and Lint Fix

Had to run pylint.

* Adding tests to ignore name

Two gates with different names are still equal, added tests for that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants