-
Notifications
You must be signed in to change notification settings - Fork 615
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
Importing the default.tensor
in PL from the lightning.tensor
device in PL lightning
#5699
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5699 +/- ##
==========================================
- Coverage 99.68% 99.67% -0.01%
==========================================
Files 415 416 +1
Lines 38898 38745 -153
==========================================
- Hits 38774 38620 -154
- Misses 124 125 +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. I don't think it makes sense to have simulate
, _apply_operation
, measurement
, _get_measurement_function
, expval
, var
, _local_expectation
in the DefaultTensor
class. In the original implementation, all these methods were implemented on a separate QuimbMPS
class, which I prefer as it creates a stronger separation of responsibilities. Moreover, I think it would make more sense as I believe that such a separation will become more necessary for readability once we start adding exact TN functionality.
…hich now is a positional argument)
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 this, but for all the side-effect-style methods, can we add a section to the docstrings:
Side Effect:
``self._circuit_MPS`` is updated to include the inputted operator
I know "Side Effect" isn't an official sphinx thing, but I think this would dramatically improve maintainability we start to include it in places like this.
@mudit2812 As discussed before, I'm creating a story to eventually move some functions in separate files in the next epic (when the time to implement the remaining method comes). @albi3ro Yep, I'll add that in the PR implementing the documentation for |
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.
👍 Thanks for addressing my comments. It's an approval from me :)
…o default_tensor_device
…I/pennylane into default_tensor_device
**Context:** We finalize the documentation for the new `default.tensor` device, introduced in #5699. **Description of the Change:** As above. **Benefits:** Documentations with usage examples are required to show users how to use the new quantum device. **Possible Drawbacks:** None, as we are simply adding documentation to an existing quantum device. **Related GitHub Issues:** None. **Related Shortcut Story** [sc-62925] --------- Co-authored-by: albi3ro <[email protected]>
Context: This PR moves part of the
lightning.tensor
device from PennyLane lightning to PennyLane. Such a device was introduced originally in the following pull requests: 1 and 2. Due to requirements change, it has been decided to create an additionaldefault.tensor
device and move it to the PennyLane repository. Specifically, we move thequimb
interface and the corresponding Python implementation oflightning.tensor
intodefault.tensor
. For reference, the PR removing such a component fromlightning.tensor
is here.Please note that the full documentation for such a device will be added in a following PR (#5719)
Description of the Change: As above.
Benefits: Having two separate devices simplifies the distinction between the C++ and the Python components. The relationship between
lightning.tensor
anddefault.tensor
will be similar to that betweenlightning.qubit
anddefault.qubit
.Possible Drawbacks: None with the existing code, since this is a new quantum device.
Related GitHub Issues: None.
Related Shortcut Stories:
[sc-63256]
[sc-62979]