-
Notifications
You must be signed in to change notification settings - Fork 600
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 CircuitDrawer single wire multiple observables #1353
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #1353 +/- ##
=======================================
Coverage 98.27% 98.28%
=======================================
Files 150 150
Lines 11262 11285 +23
=======================================
+ Hits 11068 11091 +23
Misses 194 194
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.
Thanks @antalszava! Have left some relatively minor thoughts and questions.
pennylane/circuit_graph.py
Outdated
# Initialize to None everywhere | ||
observables[wire] = [None] * max_obs_per_wire | ||
|
||
def is_returned_observable(op): |
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.
Is there a reason this helper function is nested/using closure? If not, you could just define it top level at the top of the module file (_is_returned_obs
) to avoid the overhead of creating the function within the loop.
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.
Right! Moved it to the top.
pennylane/circuit_graph.py
Outdated
def compute_max_obs_per_wire(self): | ||
"""Computes the maximum number of observables defined per wire. |
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.
Two thoughts when reading this:
-
I would prefer a shorter name for this method; the
compute
seems a bit redundant. Maybe something likemax_concurrent_measurements
? Measurements is more general than observable. -
Maybe the summary docstring could be made a bit more clear, it took me a couple of times to read it to realise what it meant
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.
Thanks! Went with max_measure_per_wire
, how would that sound? Also adjusted the docstring.
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.
To be honest, I find the 'per wire' a bit confusing, since it sounds like it will be returning a list (with each element corresponding to a wire).
Something like 'max_simultaneous_measurements` is much more identifiable to me?
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.
Sounds good, changed to that! Thanks :)
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.
Thanks Antal! Happy to approve now, but recommend renaming the method to avoid the 'per_wire' terminology
Co-authored-by: Josh Izaac <josh146@gmail.com>
Context
QNodes can have multiple measurements on the same wire so long as they are part of qubit-wise commuting observables. Drawing such QNodes results in an error, unfortunately.
This boils down to how the
CircuitGraph
processes observables. It assumes that there is a single observable for every QNode or alternatively it merges observables for separate return measurement processes. When there are more than one observable per wire, it causes an errorChanges
Changes the logic in the
greedy_layers
method of theCircuitGraph
class to account for multiple cases where there was a wire that was measured more than once.Note: in such cases the circuit includes diagonalizations and the observables will be drawn as PauliZ operators.
Ralated Issues
Closes #1326