-
Notifications
You must be signed in to change notification settings - Fork 56
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
new torontonian sampling #248
Conversation
…ction which includes displacement@
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 1263 1260 -3
=========================================
- Hits 1263 1260 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as far as I can tell. Mainly checked it code-quality wise.
thewalrus/tests/test_samples.py
Outdated
@@ -429,6 +453,7 @@ def test_thermal_state_torontonian(self, sample_func): | |||
@pytest.mark.parametrize("parallel", [True, False]) | |||
def test_torontonian_sample_graph(self, parallel): | |||
"""Test torontonian sampling from a graph""" | |||
np.random.seed(42) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this is needed here? I noticed that there's also a seed
function imported on line 28, that basically does the exact same thing. Just for consistency maybe that could be used here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We put it in whilst debugging some things. Tests were passing on my PC but not in the CI. I'll try to see if I can get the CI tests to pass without the seed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, tests seem to pass without the seed! :D
Context:
the old torontonian sampling function using tor, and so could not be used for states with displacement. It was also a little clumsy.
Description of the Change:
The function now calls
threshold_detection_prob
, making the code a bit cleaner and importantly also allows for displaced Gaussian states to be sampled from.Benefits:
Cleaner code, nonzero displacement now allowed.
Possible Drawbacks:
We could do some clever things to speed it up slightly.
Related GitHub Issues: