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

modified __add__ for Observable and Hamiltonian to support adding 0 #2603

Merged
merged 12 commits into from
Jun 3, 2022

Conversation

Qi-Hu
Copy link
Contributor

@Qi-Hu Qi-Hu commented May 22, 2022

Context:

We would like to be able to sum over a list of observables as follows:

H = sum([qml.PauliX(i) for i in range(10)])

Since sum starts at integer 0 by default, we need to define addition between observables and 0.

Description of the Change:

We modified the __add__ method for Observable and Hamiltonian classes, so that integer 0 can be added to those objects, either from the left side or the right side. Unit tests were added for this feature.

Benefits:

Now we can directly sum a list of observables in an elegant way.

Possible Drawbacks:

None.

Related GitHub Issues:

Closes #2423.

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #2603 (8d9d442) into master (2b07d22) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2603   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files         248      248           
  Lines       19975    19985   +10     
=======================================
+ Hits        19892    19902   +10     
  Misses         83       83           
Impacted Files Coverage Δ
pennylane/operation.py 96.59% <100.00%> (+0.02%) ⬆️
pennylane/ops/qubit/hamiltonian.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b07d22...8d9d442. Read the comment docs.

@co9olguy
Copy link
Member

Hi @Qi-Hu, let us know when the PR is ready for review, or if you have any questions 🙂

@Qi-Hu
Copy link
Contributor Author

Qi-Hu commented May 24, 2022

Hi @Qi-Hu, let us know when the PR is ready for review, or if you have any questions 🙂

Hi @co9olguy, this PR is ready for review. Thanks!

@co9olguy co9olguy requested a review from antalszava May 24, 2022 18:08
Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Hi @Qi-Hu, the core solution is looking great overall! 🎉 🙂 Great to have support for adding 0 to observables both from the left side and the right side.

There's one subtlety that would be great to adjust and a couple of additional things to test.

  1. The in-place summation (__iadd__ method) of the qml.Hamiltonian class has a different implementation to the regular addition (__add__ method).

Although not explicitly mentioned in the original issue, attempting to add 0 to a Hamiltonian object using in-place addition raises an error:

H = sum([qml.PauliX(i) for i in range(10)])
H += 0
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-14-2918b18b0578> in <module>
----> 1 H += 0

~/pennylane/pennylane/ops/qubit/hamiltonian.py in __iadd__(self, H)
    620             return self
    621 
--> 622         raise ValueError(f"Cannot add Hamiltonian and {type(H)}")
    623 
    624     def __imul__(self, a):

ValueError: Cannot add Hamiltonian and <class 'int'>
  1. As for testing, it would be great to add a couple of further cases:

A. Checking two remaining qubit observables:

  • SparseHamiltonian
  • Projector

B. Testing the in-place addition support via the += operator for a few general observables.

C. Testing that the feature works for continuous-variable (CV) observables too. These observables would subclass CVObservable.

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/ops/qubit/hamiltonian.py Show resolved Hide resolved
tests/test_operation.py Show resolved Hide resolved
@Qi-Hu
Copy link
Contributor Author

Qi-Hu commented May 26, 2022

Hi @antalszava, thanks a lot for your detailed comments and useful suggestions! I have made a few changes according to your suggestions.

I have two questions:

  1. The __iadd__ method is only explicitly defined for the Hamiltonian class. Do we need to test it for general observables?
  2. I added more test cases for the __add__ method of Observable class, including SparseHamiltonian, Projector and CVObservable. However, there were a few test failures. They seemed to be caused by the same issue, which is related to the attribute 'tobytes'. I haven't figured out why this happened, but I will continue working on it.
pennylane/operation.py:1635: in compare
    return other._obs_data() == self._obs_data()
pennylane/operation.py:1598: in _obs_data
    parameters = tuple(param.tobytes() for param in ob.parameters)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

.0 = <list_iterator object at 0x7fc90b25a850>

>   parameters = tuple(param.tobytes() for param in ob.parameters)
E   AttributeError: 'float' object has no attribute 'tobytes'

pennylane/operation.py:1598: AttributeError

@Qi-Hu
Copy link
Contributor Author

Qi-Hu commented May 30, 2022

Hi @antalszava, I realized the compare method is not supported by a few classes: Projector, SparseHamiltonian, QuadOperator and FockStateProjector. To be more specific, their parameters do not have tobytes attribute. I do not know if this is an issue that needs to be fixed, so I just didn't include these test cases in my most recent commit.

@Qi-Hu Qi-Hu requested a review from antalszava May 31, 2022 17:59
Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Hi @Qi-Hu, this looks good to me! 👍 It will be very useful to have this in. 🙂

Apologies for the delay here, wasn't feeling completely well and then it has been a bit busy.

Thank you for addressing the suggestions.

I realized the compare method is not supported by a few classes: Projector, SparseHamiltonian, QuadOperator and FockStateProjector. To be more specific, their parameters do not have tobytes attribute. I do not know if this is an issue that needs to be fixed, so I just didn't include these test cases in my most recent commit.

Good catch! This should be good as such indeed. 👍

@Qi-Hu
Copy link
Contributor Author

Qi-Hu commented Jun 3, 2022

Thanks @antalszava!, please let me know if there is anything else I could do.

@antalszava antalszava merged commit 43fc4f6 into PennyLaneAI:master Jun 3, 2022
@antalszava
Copy link
Contributor

Sure 🙂 Merged it, thank you for this contribution! 🎉

@Qi-Hu
Copy link
Contributor Author

Qi-Hu commented Jun 3, 2022

Sure 🙂 Merged it, thank you for this contribution! 🎉

Glad to hear that! Thank you for your help!

@co9olguy
Copy link
Member

co9olguy commented Jun 3, 2022

🎉

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.

Support adding observable to scalar 0?
3 participants