XanaduAI / thewalrus

A library for the calculation of hafnians, Hermite polynomials and Gaussian boson sampling.
https://the-walrus.readthedocs.io
Apache License 2.0
99 stars 54 forks source link

Replace samples.py functions #338

Closed benjaminlanthier closed 1 year ago

benjaminlanthier commented 2 years ago

Context:

Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:

thisac commented 2 years ago

sc-13838

codecov[bot] commented 2 years ago

Codecov Report

Merging #338 (9b29fbd) into master (3e5b60f) will not change coverage. The diff coverage is 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #338 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 24 25 +1 Lines 1761 1853 +92 ========================================= + Hits 1761 1853 +92 ``` | [Impacted Files](https://codecov.io/gh/XanaduAI/thewalrus/pull/338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI) | Coverage Δ | | |---|---|---| | [thewalrus/loop\_hafnian\_batch.py](https://codecov.io/gh/XanaduAI/thewalrus/pull/338/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-dGhld2FscnVzL2xvb3BfaGFmbmlhbl9iYXRjaC5weQ==) | `100.00% <100.00%> (ø)` | | | [thewalrus/loop\_hafnian\_batch\_gamma.py](https://codecov.io/gh/XanaduAI/thewalrus/pull/338/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-dGhld2FscnVzL2xvb3BfaGFmbmlhbl9iYXRjaF9nYW1tYS5weQ==) | `100.00% <100.00%> (ø)` | | | [thewalrus/samples.py](https://codecov.io/gh/XanaduAI/thewalrus/pull/338/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-dGhld2FscnVzL3NhbXBsZXMucHk=) | `100.00% <100.00%> (ø)` | | | [thewalrus/\_\_init\_\_.py](https://codecov.io/gh/XanaduAI/thewalrus/pull/338/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-dGhld2FscnVzL19faW5pdF9fLnB5) | | | ------ [Continue to review full report at Codecov](https://codecov.io/gh/XanaduAI/thewalrus/pull/338?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/XanaduAI/thewalrus/pull/338?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). Last update [3e5b60f...9b29fbd](https://codecov.io/gh/XanaduAI/thewalrus/pull/338?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI).
sduquemesa commented 2 years ago

Hi @benjaminlanthier! It seems that your tests for the hafnian_repeated function are not covering the case in which A is close to zero and loop==False.

image

You can look at the details on codecov.

Make sure to format the code as well using black.

JQZ1111 commented 2 years ago

Hi @benjaminlanthier! It seems that your tests for the hafnian_repeated function are not covering the case in which A is close to zero and loop==False.

image

You can look at the details on codecov.

Make sure to format the code as well using black.

Hey @sduquemesa , about the new files loop_hafnian_batch.py and loop_hafnian_batch.gamma.py, codecov says the content of _calc_loop_hafnian_batch_even for example is not covered, yet it says the content of add_batch_edges_even, a function called withing _calc_loop_hafnian_batch_even is covered, like here: 1 2 So we wonder why is this happening? If a test accesses a function B withing function A, should function A not be covered as well?

sduquemesa commented 2 years ago

Looking better @benjaminlanthier! 💪

However, it seems that you're still missing a test for the hafnian_repeated function where A is close to zero and loop==False. See line 930 in here.

sduquemesa commented 2 years ago

Hi all, great work! 🚀

Before doing a thorough review, could you please fix the formatting so that the tests passes and fill the PR description?

sduquemesa commented 2 years ago

Please update the description of this PR before merging!

JQZ1111 commented 2 years ago

Please update the description of this PR before merging!

Sorry for the late reply. I can't seem to modify the PR description... Do you think it would be possible to close this PR and recreate another one with the right description?

sduquemesa commented 2 years ago

@JQZ1111, no need to create a new PR at all, just ask @benjaminlanthier to modify the description and that's it.

sduquemesa commented 2 years ago

And don't forget to add a new entry to the CHANGELOG file

JQZ1111 commented 2 years ago

Hello @sduquemesa, we have made some changes to the changelog. Do you think it is ok to merge?

sduquemesa commented 2 years ago

Thanks all for your contribution! 🚀 This is ready to merge. Feel free to push the green button.

nquesada commented 2 years ago

Would it be possible to run the tests of SF with this development branch to make sure nothing brakes?

sduquemesa commented 2 years ago

Thanks @nquesada for your suggestion! Indeed there are some test failing from the strawberryfields side, for example tracebacks looks like

_______________________ test_VGBS_integration[False-3-7] _______________________
Traceback (most recent call last):
  File "/home/runner/work/strawberryfields/strawberryfields/tests/apps/train/test_param.py", line 301, in test_VGBS_integration
    samples = gbs.generate_samples(A_prime, n_samples)
  File "/home/runner/work/strawberryfields/strawberryfields/strawberryfields/apps/train/param.py", line 276, in generate_samples
    samples = thewalrus.samples.hafnian_sample_state(cov, n_samples, hbar=sf.hbar, **kwargs)
  File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/thewalrus/samples.py", line 360, in hafnian_sample_state
    return _hafnian_sample(params)
  File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/thewalrus/samples.py", line 310, in _hafnian_sample
    result = generate_hafnian_sample(
  File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/thewalrus/samples.py", line 249, in generate_hafnian_sample
    det_outcome_i = np.random.choice(det_outcomes, p=probs)
  File "mtrand.pyx", line 935, in numpy.random.mtrand.RandomState.choice
ValueError: probabilities contain NaN
----------------------------- Captured stderr call -----------------------------
/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/thewalrus/samples.py:111: RuntimeWarning:
invalid value encountered in sqrt
_______________________ test_VGBS_integration[True-2-9] ________________________
Traceback (most recent call last):
  File "/home/runner/work/strawberryfields/strawberryfields/tests/apps/train/test_param.py", line 301, in test_VGBS_integration
    samples = gbs.generate_samples(A_prime, n_samples)
  File "/home/runner/work/strawberryfields/strawberryfields/strawberryfields/apps/train/param.py", line 272, in generate_samples
    samples = thewalrus.samples.torontonian_sample_state(
  File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/thewalrus/samples.py", line 573, in torontonian_sample_state
    return _torontonian_sample(params)
  File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/thewalrus/samples.py", line 523, in _torontonian_sample
    result = generate_torontonian_sample(
  File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/thewalrus/samples.py", line 461, in generate_torontonian_sample
    det_outcome = np.random.choice(det_outcomes, p=probs_k)
  File "mtrand.pyx", line 935, in numpy.random.mtrand.RandomState.choice
ValueError: probabilities contain NaN

You can view the output of the checks on the linked PR above or in this link. This issues should be addressed before merging to main.