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

Implement custom comparison for ControlledSequence #4829

Merged
merged 10 commits into from Nov 17, 2023

Conversation

EmilianoG-byte
Copy link
Contributor

@EmilianoG-byte EmilianoG-byte commented Nov 12, 2023

Context:

Currently, qml.equal does not have a registered function that compares two ControlledSequence objects correctly. This PR addresses this issue.

Description of the Change:

Registers a new overloaded implementation of _equal for arguments of type ControlledSequence. For two objects of this type, qml.equal compares if:

  • their control properties are the same
  • their base's are the same to the requested tolerances and parameter checks.

For example:

op1 = qml.ControlledSequence(qml.PauliX(0), control=1)
op2 = qml.ControlledSequence(qml.PauliX(0), control=1)
qml.equal(op1, op2)

>>> True

Benefits:

Correct comparison of two ControlledSequence objects is now possible.

Possible Drawbacks:

None.

Related GitHub Issues:

Closes: #4797

@EmilianoG-byte
Copy link
Contributor Author

EmilianoG-byte commented Nov 12, 2023

Hi!

I have based this PR on an older commit 970d30c (from 26.10) since the newest commit was throwing some import errors when I pulled from master, and this was one of the last ones were the CI checks were passing. I see the only conflict is with the changelog, so hopefully doing it this way was ok :)

Let me know what you think about my changes!

@rmoyard
Copy link
Contributor

rmoyard commented Nov 13, 2023

Hello @EmilianoG-byte , thanks for your contribution! I will give an initial review of your PR this afternoon.

@rmoyard rmoyard self-requested a review November 13, 2023 18:31
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thank you @EmilianoG-byte your PR looks good 💯 I have left some comments.

Concerning your import issue, I think it might be that we added pytest-benchmark mandatory to run the tests suite, you should pip install the package and let us know if the issue persists.

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
tests/ops/functions/test_equal.py Outdated Show resolved Hide resolved
tests/ops/functions/test_equal.py Show resolved Hide resolved
pennylane/ops/functions/equal.py Outdated Show resolved Hide resolved
pennylane/ops/functions/equal.py Outdated Show resolved Hide resolved
EmilianoG-byte and others added 2 commits November 13, 2023 21:36
Co-authored-by: Romain Moyard <rmoyard@gmail.com>
Co-authored-by: Romain Moyard <rmoyard@gmail.com>
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (02295e4) 99.66% compared to head (a4744cb) 99.65%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4829      +/-   ##
==========================================
- Coverage   99.66%   99.65%   -0.01%     
==========================================
  Files         382      382              
  Lines       34465    34211     -254     
==========================================
- Hits        34348    34093     -255     
- Misses        117      118       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@lillian542 lillian542 left a comment

Choose a reason for hiding this comment

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

This looks good @EmilianoG-byte, thanks for the thorough tests! 🚀

@EmilianoG-byte
Copy link
Contributor Author

This looks good @EmilianoG-byte, thanks for the thorough tests! 🚀

Thank you 😃 @lillian542 !!

@EmilianoG-byte
Copy link
Contributor Author

Hi @rmoyard, I made the changes we discussed 😊
Let me know if some of them were still up for discussion or if they are okay!

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 💯

@EmilianoG-byte
Copy link
Contributor Author

thank you @rmoyard and @lillian542!

Two final things:
1- Regarding the issue I mentioned, it turns out it was a problem on my side. I was using an environment with python 3.8, so I upgraded to 3.11 and everything was fine! The pipeline on the master branch was failing in most recent commits tho, is that okay?

2- Any thoughts on my comment regarding the parametrisation of the first functions? In general, I wanted to know if using pytest parametrize like that was a standard/acceptable practice. I have never seen it being used like that, i.e. having the parameter be the function to test:

@pytest.mark.parametrize("test_class", [Controlled, ControlledSequence])

@rmoyard
Copy link
Contributor

rmoyard commented Nov 17, 2023

Hi @EmilianoG-byte, I am glad you solved the issue! For the second part, in general you are welcome to use it but I also like when it is tested separated. It makes it easier to refactor if needed.

@Alex-Preciado Alex-Preciado changed the title Implement custom comparison for ControlledSequence (Residency program) Implement custom comparison for ControlledSequence Nov 17, 2023
@Alex-Preciado
Copy link
Contributor

Awesome! Thank you so much @EmilianoG-byte. Merging this now 😎

@Alex-Preciado Alex-Preciado merged commit 68b2f67 into PennyLaneAI:master Nov 17, 2023
33 checks passed
@EmilianoG-byte
Copy link
Contributor Author

Awesome! Thank you so much @EmilianoG-byte. Merging this now 😎

Thank you @Alex-Preciado 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement custom comparison for ControlledSequence
4 participants