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

Add Variational Quantum Algorithms Module #123

Merged
merged 58 commits into from
Apr 30, 2022
Merged

Conversation

EnBr55
Copy link
Contributor

@EnBr55 EnBr55 commented Feb 9, 2022

Functionality as outlined in #118

  • Core functionality
  • Following style requirements
  • API documentation
  • Tests
    • Mostly done, still in progress
  • Manual documentation
    • In progress

This PR introduces a new module, qutip_qip.vqa, for defining and simulating Variational Quantum Algorithms. For examples for how this module could be used, please see https://github.com/EnBr55/qutip-vqa-examples/

Co-authored-by: Nathan Shammah <nathan.shammah@gmail.com>
@BoxiLi
Copy link
Member

BoxiLi commented Apr 17, 2022

It is a bit annoying that the test needs to be approved every time when a new push is made. Not sure if there is a way to just allow any future tests to run for this PR. Sorry for all the delay, @EnBr55 Ben, please feel free to drop me an email next time you add something and want to run the test. Sometimes I miss the GitHub email and am unaware of any new push.

We can also try to get this PR merged first and add new patches in a different one so that any future PR of yours won't need approval.

The doc test is failing because the online doc generator does not have latex installed. You can render a png locally and add it as a figure, just like the other part of doc.

@EnBr55 EnBr55 marked this pull request as ready for review April 19, 2022 08:31
Copy link
Member

@BoxiLi BoxiLi left a comment

Choose a reason for hiding this comment

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

@EnBr55 This is just part of the review. The code looks very neat. Mostly just naming plus a few of my questions, to clarify things. You don't have to accept all of them and feel free to discuss :)

src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
doc/source/qip-vqa.rst Show resolved Hide resolved
doc/source/qip-vqa.rst Outdated Show resolved Hide resolved
Copy link
Member

@BoxiLi BoxiLi left a comment

Choose a reason for hiding this comment

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

@EnBr55 Here is the second half of the review. Sorry for the delay.

  • There are two png figures with the same content. One is duplication I guess (doc/circ.png)?

  • If it is not too late, can we rename the public API
    n_qubits -> num_qubits
    n_layers -> num_layers
    n_paramters -> num_parameters
    to be consistent with the rest of the naming convention in qutip-qip? For the intermediate result not accessible to the user it does not matter.

  • It is fine to merge as is. If you want to develop further on this module. I would recommend to define different types of VQA blocks as subclasses. In that way you don't have to use lots of if-else branches to check the input. You only need to specify this when defining the VQA blocks, e.g. UnitaryVQABLock, HamiltonianVQABlock etc..

src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
doc/source/qip-vqa.rst Outdated Show resolved Hide resolved
doc/source/qip-vqa.rst Outdated Show resolved Hide resolved
doc/source/qip-vqa.rst Outdated Show resolved Hide resolved
src/qutip_qip/vqa.py Outdated Show resolved Hide resolved
@BoxiLi BoxiLi merged commit 2f2879e into qutip:master Apr 30, 2022
@BoxiLi
Copy link
Member

BoxiLi commented Apr 30, 2022

Congrats @EnBr55 ! Feel free to open further PRs for more documentation examples as well as further development!

@nathanshammah Pinging Nathan to share the moment.

@BoxiLi BoxiLi added this to the qutip-qip-0.3.0 milestone Apr 30, 2022
@EnBr55
Copy link
Contributor Author

EnBr55 commented Apr 30, 2022

Thanks so much for all the help and advice @BoxiLi @nathanshammah !! :)

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.

Add Variational Quantum Algorithms Module
3 participants