-
Notifications
You must be signed in to change notification settings - Fork 61
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
fixing CI #420
fixing CI #420
Conversation
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 to me and CI seems to pass.
@@ -12,7 +12,7 @@ | |||
|
|||
def test_parallel_circuit_evaluation(backend): | |||
"""Evaluate circuit for multiple input states.""" | |||
if 'GPU' in qibo.get_device() or os.name == 'nt': # pragma: no cover | |||
if 'GPU' in qibo.get_device() or os.name == 'nt' or qibo.get_backend() == "tensorflow": # pragma: no cover |
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.
Optionally we could also cover the error cases here using:
with pytest.raises(RuntimeError):
...
instead of pytest.skip
. This way we could remove # pragma: no cover
from the errors above and include them in the coverage check.
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 was thinking about it, however then we need to wrap the parallel function call in a if/else condition, right? (and duplicate the test basically)
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.
Yes, I am not sure if it is the best way but this is what I do in a few other tests where I check for similar errors. For these tests I think it is sufficient to do something like:
if 'GPU' in qibo.get_device() or os.name == 'nt' or qibo.get_backend() == "tensorflow":
with pytest.raises(RuntimeError):
r2 = parallel_execution(c, states=states, processes=2)
else:
r2 = parallel_execution(c, states=states, processes=2)
np.testing.assert_allclose(r1, r2)
so the code duplication is not that large. Not very necessary to do this though.
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.
Yeah, let's keep as it is, thanks.
Codecov Report
@@ Coverage Diff @@
## updocs112 #420 +/- ##
=============================================
Coverage ? 100.00%
=============================================
Files ? 79
Lines ? 11401
Branches ? 0
=============================================
Hits ? 11401
Misses ? 0
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Fixes #418.