Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactored and created an abstraction for control values #5362
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
Refactored and created an abstraction for control values #5362
Changes from 7 commits
ec76158
45f476d
172afa1
5e717fe
b22255d
b2fbd50
1e89a28
0f14dce
d0b0b67
989af8a
5bc5194
a4629b8
a62b520
da19236
98e92fe
193a404
7bb3df5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The docstring should be more detailed. For example:
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.
PTAL
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.
This assumes that
self._internal_representation
andother._internal_representation
can be simply added together. But their types areAny
, so this need not be true.Does the current implementation assume that
ProductOfSums
is the only derived type, and would be special cased later once we add more derived types?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, this is just for refactoring, this function will be rewritten in a better way once the linked structure is introduced
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.
Please explain more what the "expanded" representation means / contains. Does it depend upon what internal representation the subclasses use? Does every tuple correspond to a "one value per qubit" representation?
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.
PTAL
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.
Improve docstrings, it's not clear to me what it means to "have an identifier from which object can be rebuilt". Does this mean an identifier we can use for serialization / deserialization ?
Also, when would a user use this method? Do we need this to be part of the public API ? Can we just enforce that the class should have a
__repr__
defined that satisfieseval(repr(cv)) == cv
?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 changed the docstring, this function essentially returns the internal representation, I changed it to be private and I will remove it in the next PR, it's now used in functions in controlled_gate and controlled_operation that require access to the internal representation