-
Notifications
You must be signed in to change notification settings - Fork 604
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
Add and Multiply operations for Resource objects #6567
base: master
Are you sure you want to change the base?
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6567 +/- ##
==========================================
- Coverage 99.29% 99.29% -0.01%
==========================================
Files 454 454
Lines 43270 43332 +62
==========================================
+ Hits 42966 43027 +61
- Misses 304 305 +1 ☔ View full report in Codecov by Sentry. |
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.
Looks good, just a few small comments 👍🏼. Also missing a changelog entry.
@@ -14,6 +14,9 @@ | |||
""" | |||
Stores classes and logic to aggregate all the resource information from a quantum workflow. | |||
""" | |||
from __future__ import annotations |
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.
from __future__ import annotations |
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 needed for the type hints to work. Is there a problem with this import?
Actually looking at the docs, it would be nice to add examples in the doc-string for each of the functions. The information in the docstring (although sufficient) feels empty. |
Also you haven't updated the |
def _add_shots(s1: Shots, s2: Shots) -> Shots: | ||
if s1.total_shots is None: | ||
return s2 | ||
|
||
if s2.total_shots is None: | ||
return s1 | ||
|
||
shot_vector = [] | ||
for shot in s1.shot_vector + s2.shot_vector: | ||
shot_vector.append((shot.shots, shot.copies)) | ||
|
||
return Shots(shots=shot_vector) |
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.
Looks good, I just wonder if it's ideal to have this defined here, or better in pennylane/measurements/shots.py
as part of the Shots
class.
Probably best to check with Soran.
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 was thinking the same thing, but I didn't want to add an unrelated file to the PR. I'll ask Soran.
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 current location looks good to me. @Jaybsoni please also check.
This PR adds
add_in_series
,add_in_parallel
,mul_in_series
, andmul_in_parallel
for theResource
objects. [sc-77943].