-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix bug causing slices with depth > 1 to be forward-prop'd #62
Conversation
This is a bit tangential, but in adjusting the unit tests I realized this line should maybe be |
Pull Request Test Coverage Report for Build 13681086388Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
I agree, it should be |
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.
These look good
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.
Just some minor comments, other than that, this LGTM 👍
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
@@ -164,7 +164,8 @@ def handle_timeout(signum, frame): | |||
# PERF: we will likely need to parallelize this loop | |||
for i in range(num_observables): | |||
non_trivial_slice = False | |||
for op_idx, op_node in enumerate(circuit_to_dag(slice_).topological_op_nodes()): | |||
op_nodes = list(circuit_to_dag(slice_).topological_op_nodes())[::-1] |
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.
@mrossinek I'll just cast to list here and reverse it. That way I can get its length for logger
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 guess that is fine. Although I think it would have been fine if the logger counted forwards
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.
Well reversed
cant act on a generator, so it was going to need to be cast either way in order to iterate backwards (as far as I can tell, at least). We can adjust the logger before release if you want it to behave differently
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 we could do:
enumerate(reversed(...))
But as I said, it is not a big deal 👍
Fixes #60