-
Notifications
You must be signed in to change notification settings - Fork 37
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
Support qml.counts
for circuits
#267
Conversation
Hi @king-p3nguin, thanks for starting your contribution! Could you also add unit tests to this PR? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 1211 1229 +18
Branches 295 304 +9
=========================================
+ Hits 1211 1229 +18 ☔ View full report in Codecov by Sentry. |
Hi @king-p3nguin, the basic use case from the issue throws error
with error message And another use case
throws a different error |
Also, could you add more tests to increase the coverage? thanks |
@yitchen-tim Thank you for checking. The two basic use cases did not show errors in my environment. Can you ensure the code is executed within my fork's (As for unit testing, please give me some time as it seems to be quite complicated to create a mock) |
@king-p3nguin Great, thanks, I did have some dependencies issues so it was not working correctly. The basic examples works for me now. After full test coverage, we should be good to go! Also, per UnitaryHack guideline, it's good as long as PR is opened before EOD today and merged before 6/19. So we still have a week. |
return Sample(braket_observable, targets) | ||
else: | ||
raise NotImplementedError(f"Unsupported return type: {return_type}") | ||
raise NotImplementedError(f"Unsupported return type: {return_type}") # pragma: no cover |
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 add test case for unsupported return type rather than # pragma: no cover
?
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.
It seems that when an observable is defined, there are only four possible types (Expectation
, Variance
, Sample
, and Counts
), so I will just use the else statement here.
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 else statement is prone to error. Pennylane supports few other return types, and the else statement doesn't really handle all these other result types.
To add coverage and test the unsupported return type for these lines
else:
raise NotImplementedError(f"Unsupported return type: {return_type}")
, you could mock the measurement
as something like
measurement = Mock(
return_type = Enum('ObservableReturnTypes', {'Foo': "foo"}).Foo,
observable = qml.PauliZ()
)
If any function that is imcompatible with the mocked measurement
, you can adjust the mock or patch the imcompatible function.
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.
Thank you for the suggestion! I made the change as requested.
@yitchen-tim |
@king-p3nguin thanks, coverage is 100%, woot! Just one more change around testing unsupported result 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.
Great work! Thanks again for the contribution @king-p3nguin
Issue #, if available:
#195
Description of changes:
qml.counts()
is now supported.Testing done:
Example:
output:
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.