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
Dagger() * IdentityOperator() now simplifies by default #19783
Dagger() * IdentityOperator() now simplifies by default #19783
Changes from 1 commit
bfb4989
c528041
d91e3fe
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.
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 superclass of
Operator
andDagger
that would make more sense here? ShouldDagger
be a subclass ofOperator
?I don't know the quantum code so well but special casing types like this suggests that things can be better organised somehow.
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 agree things can be better organized. Also: not every functionality is tested properly.
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 is exactly what I was thinking and suggested on the mailing list. But while working on it, it turns out if such a change is made
print
doesn't work properly.print(Dagger(A))
givesA
. I think this happens due to calling of_print_contents
ofOperator
. And due to unfamiliarity with the printing module I found it difficult to further look into itThere 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 think that if
Dagger
is a subclass ofOperator
then if you add a_print_Dagger
method it will take precedence over_print_Operator
.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.
Apologies for not being able to reply earlier, I tried overriding
_print_contents()
and made a separate_print()
function for dagger but still it didn't work.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 you show us the diff? It would be easier to comment after that. I think it should work if you add a new
_print_Dagger()
method.