-
Notifications
You must be signed in to change notification settings - Fork 595
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
Raise an error or warning in qml.compile if basis_set contains operations instead of strings #6137
base: master
Are you sure you want to change the base?
Raise an error or warning in qml.compile if basis_set contains operations instead of strings #6137
Conversation
Signed-off-by: Anurav Modak <[email protected]>
Signed-off-by: Anurav Modak <[email protected]>
Signed-off-by: Anurav Modak <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6137 +/- ##
==========================================
- Coverage 99.67% 99.66% -0.02%
==========================================
Files 432 445 +13
Lines 41839 42363 +524
==========================================
+ Hits 41702 42220 +518
- Misses 137 143 +6 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Anurav Modak <[email protected]>
Signed-off-by: Anurav Modak <[email protected]>
Signed-off-by: Anurav Modak <[email protected]>
Signed-off-by: Anurav Modak <[email protected]>
ddb8c08
to
4ec8cbe
Compare
Signed-off-by: Anurav Modak <[email protected]>
c358d6e
to
bbc1c4d
Compare
Hey @albi3ro , i have added the required code changes and mandatory tests, to throw an error for the prescribed scenario, kindly review it and let me know if any thing else has to be done on coding or implementation part! |
Thanks for opening this PR @AnuravModak . Sorry if there was confusion on the issue, but we don't currently support to contain operator types like:
Part of the suggestion was to start allowing such a syntax. Do you think you could expand the scope of the PR to include that change? If not, we should also raise an error if |
Signed-off-by: Anurav Modak <[email protected]>
6185640
to
9d212f8
Compare
Hey @albi3ro , thanks for clarifying! Now the compile transform will handle both operation names and types within the basis_set. let me know if it matches your expectations! |
Looks like it still doesn't accept class types as a valid way of specifying the basis set. Maybe we should limit the scope to just validating that the basis set only contains strings, and then do a follow up with the new feature allowing the specification of the basis set via class types. The feature we are looking for would be allowing:
Which we would expect to decompose the |
Signed-off-by: Anurav Modak <[email protected]>
Hi @albi3ro , I think my new changes may align closer to your requrements: Handling Class Types in basis_set:
Stopping Condition: Now also i have deployed the code in my local and have tested it! Let me know if the results are right or not!
output:
Or should i just simply implement this change that you have suggested?
|
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.
Sorry about not getting back to this quickly.
The code looks great!
For the tests, can we write a unit test for tape in -> batch, fn out? We would also want tests where the basis set is specified via types, and unit tests for the error if the basis set is invalid. The test should also cover an input tape that contains operations that should be decomposed.
Tests on the qnode/ qfunc can tend to hide a lot of incorrect behavior, since compile
does not change the result output. And in this case, we aren't even checking the value of the output. So a lot can go wrong between executing the qnode and still getting a float out. I've seen this testing anti-pattern hide a lot of bugs.
Also, the basis set in the test includes all of the operations in the qfunc. It doesn't actually check that operations outside that basis set get decomposed. We could want to make sure something like CRX
gets decomposed before the other compilation transforms get applied.
Hope that clarifies things.
Towards #6132
This PR includes the following changes:
If the basis set contains any items that are not strings or subclasses of
qml.operation.Operator
, a ValueError will be raised. Additionally, if the basis set contains operator types instead of names and results in an empty set, an error will be raised.