Skip to content
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

Merged
merged 37 commits into from
May 23, 2024

Conversation

PietropaoloFrisoni
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni commented May 16, 2024

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 additional default.tensor device and move it to the PennyLane repository. Specifically, we move the quimb interface and the corresponding Python implementation of lightning.tensor into default.tensor. For reference, the PR removing such a component from lightning.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 and default.tensor will be similar to that between lightning.qubit and default.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]

Copy link

codecov bot commented May 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.67%. Comparing base (b7b4b75) to head (a747fd7).
Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@albi3ro albi3ro self-requested a review May 21, 2024 18:39
@mudit2812 mudit2812 self-requested a review May 21, 2024 18:43
Copy link
Contributor

@mudit2812 mudit2812 left a 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.

pennylane/devices/__init__.py Outdated Show resolved Hide resolved
pennylane/devices/default_tensor.py Outdated Show resolved Hide resolved
pennylane/devices/default_tensor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@albi3ro albi3ro left a 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.

@PietropaoloFrisoni
Copy link
Contributor Author

@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 default.tensor, which still requires additional work.

Copy link
Contributor

@albi3ro albi3ro left a 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 :)

@PietropaoloFrisoni PietropaoloFrisoni merged commit 058603a into master May 23, 2024
38 checks passed
@PietropaoloFrisoni PietropaoloFrisoni deleted the default_tensor_device branch May 23, 2024 19:34
PietropaoloFrisoni added a commit that referenced this pull request May 29, 2024
**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 <chrissie.c.l@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants