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.
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
Created SumOfProducts subclass of ControlValues #5755
Created SumOfProducts subclass of ControlValues #5755
Changes from 1 commit
28d0a4d
621973d
24caf91
d325b74
b1eb59c
a5940fa
da911ac
ff4466b
24cc4d4
cd7d768
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.
Can we add a post init method which verifies that length of all nested tuples in
self._internal_representation
is the same? This is assumed to be true in the implementation of methods like_number_variables
belowThere 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.
added, I also test for the uniqueness of products in it
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.
Why is the diagram_repr not implemented?
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 most basic implementation is problematic since the strings will be quite large since they have size num_products x num_qubits => O(num_qubits x 2^num_qubits), this is why I'm hestiant to implement it, however I added it anyway :)
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.
Why is this NotImplemented? Also, I don't like the pattern of
a) Declaring abstract methods in the API, telling the users that all implementations will implement these abstract methods.
b) Returning
NotImplemented
in the implementation of the abstract methods.If we want to allow implementations to return
NotImplemented
; then these shouldn't be abstract methods in the first place.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 reason that I'm hesitant to implement it is that it will have a different meaning from that of ProductOfSums, where in ProductOfSums we get valide values of qubit, here we get a product
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.
We are not doing the the linked-list style concatenation discussed earlier? Will that come in a follow-up PR?
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 would wait until next PR, I want to introduct the SumOfProducts class before Cirq 1.0, because it has full expressive power (i.e. fixes the original issue), the linked structure can be introducted later without affecting users
the difference between the current state and what will happen when the linked structure is introduced is expressions like this ((x xor y) and (z and w)) and can now be represented by a SumOfProducts objects that has 8 products, while when we introduce the linked structure then we will only need to store 5 products (2 for the first and 3 for the second) and similarly for larger expressions where we would could in some cases store an exponenetial number of products in pseudopolynomial space.