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

SymbolicHamiltonian refactor #1548

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

BrunoLiegiBastonLiegi
Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi commented Dec 18, 2024

This should address #1494

EDIT: this turned out to be more in general a refactor of SymbolicHamiltonian and partly of hamiltonian/models.py. Namely, SymbolicHamiltonian now only has a form attribute, dense and terms are cached properties computed starting from form and, thus, they can no longer be manually setted. Concerning hamiltonian/models.py, now the models are built starting from the form and not the terms as it was before. I also made the dense representation of the models more backend aware, replacing the numpy operations with the backend ones.

Note: tests on cupy are still failing due to this qiboteam/qibojit#196 .

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi changed the title Remove SymbolicHamiltonian terms and dense setters SymbolicHamiltonian refactor Dec 19, 2024
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 93.68421% with 6 lines in your changes missing coverage. Please review.

Project coverage is 99.31%. Comparing base (9fa2fb1) to head (6cc8435).
Report is 26 commits behind head on master.

Files with missing lines Patch % Lines
src/qibo/hamiltonians/hamiltonians.py 92.85% 4 Missing ⚠️
src/qibo/hamiltonians/models.py 94.87% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1548      +/-   ##
==========================================
- Coverage   99.67%   99.31%   -0.37%     
==========================================
  Files          76       76              
  Lines       11234    11168      -66     
==========================================
- Hits        11198    11092     -106     
- Misses         36       76      +40     
Flag Coverage Δ
unittests 99.31% <93.68%> (-0.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

BrunoLiegiBastonLiegi commented Dec 20, 2024

I am still unsure whether it is worth keeping the two different ways for computing the dense form of the SymbolicHamiltonian, namely from_terms and from_form.
Therefore I've done a little benchmark on 4 different hamiltonians ("TFIM", "XXZ", "TFIM**2", "XXZ**2") with the numpy backend for qubits from 2 to 10. In particular the number of terms (after expansion) for a given number of qubits for the TFIM**2 (the biggest) is:

{
    "2q": 36,
    "3q": 81,
    "4q": 144,
    "5q": 225,
    "6q": 324,
    "7q": 441,
    "8q": 576,
    "9q": 729,
    "10q": 900,
    "11q": 1089
}

the average runtimes across 10 runs are as follows:

time vs nqubits
time_vs_nqubits.pdf

time vs nterms
time_vs_nterms.pdf

the from_terms has an advantage when the number of terms is "smaller" (less than ~40 let's say), after that the from_form seems to prevail, but, on the long run, for a large number of qubits and terms the from_terms appears to become comparable if not better.

Long story short, if I had to pick only one of the two I'd probably lean towards the from_terms, but I could also internally automatically switch between the two given a pre-defined nterms threshold.

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.

1 participant