Closed mgarnier59 closed 2 weeks ago
@mgarnier59 @thierry-martinez how do the performance of simulation compare with original implementation? I presume no significant difference since it's only modifying the initialization part?
@mgarnier59 @thierry-martinez how do the performance of simulation compare with original implementation? I presume no significant difference since it's only modifying the initialization part?
We didn't investigate it. What's more it provides more feature compared to the previous versions. I tend to think that global performance issues will be adressed later.
@mgarnier59 @thierry-martinez how do the performance of simulation compare with original implementation? I presume no significant difference since it's only modifying the initialization part?
We didn't investigate it. What's more it provides more feature compared to the previous versions. I tend to think that global performance issues will be adressed later.
I believe we should at least test it's not surprisingly slow by 2x or more, or have completely different scaling as a function of problem size, which indicates something is wrong with the code. if it's up to a few tens of % (which I believe is most likely the case) indeed it can be safely addressed later.
@mgarnier59 @thierry-martinez have you checked the examples run without errors?
@mgarnier59 @thierry-martinez how do the performance of simulation compare with original implementation? I presume no significant difference since it's only modifying the initialization part?
We didn't investigate it. What's more it provides more feature compared to the previous versions. I tend to think that global performance issues will be adressed later.
I believe we should at least test it's not surprisingly slow by 2x or more, or have completely different scaling as a function of problem size, which indicates something is wrong with the code. if it's up to a few tens of % (which I believe is most likely the case) indeed it can be safely addressed later.
I obtained these benchmarks (input-init
is the version I just pushed, which includes the last changes in master
; input-init^1
is the version before the merge). Performances are quite close indeed.
+-------------------------------------------------------------------------------------+
| statement |master|input-init|input-init^1|
+------------------------------------------------------+------+----------+------------+
| pytest.main([]) | 16.28| 16.41 | 14.67 |
+------------------------------------------------------+------+----------+------------+
|random_circuits(seed=42, times=20, nqubit=15, depth=1)| 3.12 | 3.30 | 3.27 |
+------------------------------------------------------+------+----------+------------+
|random_circuits(seed=42, times=20, nqubit=16, depth=1)| 9.69 | 8.24 | 8.45 |
+------------------------------------------------------+------+----------+------------+
|random_circuits(seed=42, times=20, nqubit=17, depth=1)| 17.61| 18.91 | 17.72 |
+------------------------------------------------------+------+----------+------------+
|random_circuits(seed=42, times=20, nqubit=18, depth=1)| 39.14| 41.15 | 42.17 |
+------------------------------------------------------+------+----------+------------+
|random_circuits(seed=42, times=20, nqubit=19, depth=1)| 72.78| 76.26 | 77.80 |
+------------------------------------------------------+------+----------+------------+
|random_circuits(seed=42, times=20, nqubit=20, depth=1)|155.43| 162.73 | 163.46 |
+-------------------------------------------------------------------------------------+
@thierry-martinez Thanks a lot, that looks great!
Attention: Patch coverage is 86.31579%
with 26 lines
in your changes missing coverage. Please review.
Project coverage is 72.26%. Comparing base (
30a3f6b
) to head (19dfd8a
).
Files | Patch % | Lines |
---|---|---|
graphix/sim/tensornet.py | 45.83% | 13 Missing :warning: |
graphix/sim/statevec.py | 93.22% | 4 Missing :warning: |
graphix/linalg_validations.py | 40.00% | 3 Missing :warning: |
graphix/sim/density_matrix.py | 93.02% | 3 Missing :warning: |
graphix/states.py | 90.62% | 3 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@EarlMilktea could you take a quick look at the changes?
Thank you very much for your review, @EarlMilktea. I believe I addressed or commented all your comments. Do you mind to check if everything is ok for you now? Thank you again!
Thank you for your suggestions/fixes, @thierry-martinez !
Could you resolve my new comment on conftest.py
?
@EarlMilktea , I fixedconftest.py
in 55331b5 according to your comments. Thanks!
@EarlMilktea , I fixed
conftest.py
in 55331b5 according to your comments. Thanks!
Hi @thierry-martinez , it looks like the recent changes you mention on this page are on separate branch, e.g. 55331b5, and is not showing up on the diff of this PR. could you take a look and push to this branch? After that, this PR should be ready for merge.
@EarlMilktea , I fixed
conftest.py
in 55331b5 according to your comments. Thanks!Hi @thierry-martinez , it looks like the recent changes you mention on this page are on separate branch, e.g. 55331b5, and is not showing up on the diff of this PR. could you take a look and push to this branch? After that, this PR should be ready for merge.
Sorry, I didn't push on the right repo! Fixed.
@EarlMilktea please check that your comments are resolved, and approve if so. From my side, this is nearly ready except the check related to TN backend above.
@thierry-martinez
I still can't see some of your changes correctly (such as SV_Data
-> SVData
).
Could you verify again that you're pushing to the right repo.?
@thierry-martinez
I still can't see some of your changes correctly (such as
SV_Data
->SVData
).Could you verify again that you're pushing to the right repo.?
All references to SV_Data
have been renamed into Data
in cabd019, except one occurrence in the doc-comment of the Statevec
class: fixed in 20d0237.
@EarlMilktea please check that your comments are resolved, and approve if so. From my side, this is nearly ready except the check related to TN backend above.
Fixed in 05aa7c9.
@thierry-martinez
Even though I still see some small problems regarding type hints, it would not affect the functionality itself.
LGTM!
@shinich1
I will submit a new PR to thoroughly fix type-related problems.
Thank you very much! Merged.
Allowing arbitrary intialization of the input nodes for statevector and density matrix backends + tests.
Several format of initialization are supported (Statevec or DensityMatrix objects, list of newly-defined State objects, numerical data). Main change is that basic states (|0>, |+>, ...) are now in states.BasicStates and no longer in ops.py.