-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update to support SF v0.11 #3
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3 +/- ##
=======================================
Coverage 100% 100%
=======================================
Files 11 5 -6
Lines 1493 299 -1194
=======================================
- Hits 1493 299 -1194
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.
Nice 👍, left a couple questions.
numeric_level = getattr(logging, logLevel.upper(), 10) | ||
# defaults | ||
TOL = 1e-2 | ||
HBAR = 2 |
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.
worthwhile using an unconventional value here (to help detect bugs)?
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.
or will you leave that up to travis?
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.
I actually changed this to HBAR = 1.7
as in SF, and discovered that a huge majority of the tests have hbar = 2
assumptions built into them 🤦♂ I didn't want to go out of scope, but I'll make a new issue to fix that.
assert np.allclose(state.fock_prob([1, 1]), exp[1], rtol=tol) | ||
assert np.allclose(state.fock_prob([0, 2]), exp[2], rtol=tol) | ||
|
||
def test_1x2_quadrature_operators(self, eng, tol): |
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.
new test?
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.
Yep, noticed it wasn't being tested previously, and GitHub/Codecov wouldn't pass the coverage checks without it
self.H = bose_hubbard(1, 2, self.J, self.U) | ||
self.Hquad = get_quad_operator(self.H, hbar=self.hbar) | ||
|
||
def test_hbar_outside_context(self): |
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.
no longer need these hbar tests?
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.
No, this is because of the change in how hbar
is handled in SF v0.11; hbar is no longer part of the engine/program context, but a separate global variable.
"""Test S2gate produces correct cov and means""" | ||
self.logTestName() | ||
# NOTE: There is currently a bug in strawberry fields, |
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.
nice!
No description provided.