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

Update lightning.tensor docs #909

Merged
merged 23 commits into from
Sep 20, 2024
Merged

Update lightning.tensor docs #909

merged 23 commits into from
Sep 20, 2024

Conversation

multiphaseCFD
Copy link
Member

@multiphaseCFD multiphaseCFD commented Sep 16, 2024

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    tests directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:

[SC-65786] and [SC-68333]

Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:

@multiphaseCFD multiphaseCFD changed the title [WIP] Update lightning.tensor docs Update lightning.tensor docs Sep 16, 2024
@multiphaseCFD multiphaseCFD added the do not merge Do not merge PR until this label is removed label Sep 16, 2024
@multiphaseCFD multiphaseCFD marked this pull request as ready for review September 16, 2024 14:14
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.33%. Comparing base (05ee305) to head (f52a43d).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (05ee305) and HEAD (f52a43d). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (05ee305) HEAD (f52a43d)
8 6
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #909       +/-   ##
===========================================
- Coverage   96.27%   56.33%   -39.94%     
===========================================
  Files         214       24      -190     
  Lines       28620     2297    -26323     
===========================================
- Hits        27553     1294    -26259     
+ Misses       1067     1003       -64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AmintorDusko AmintorDusko added the urgent Mark a pull request as high priority label Sep 18, 2024
Copy link
Contributor

@tomlqc tomlqc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @multiphaseCFD!
Just few suggestions about README vs device.rst

README.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@Qottmann Qottmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @multiphaseCFD !

I am bit confused about some of the recommendations you are making, perhaps I am missing something so it would be good to clarify that to me (and to the user)

.github/CHANGELOG.md Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Show resolved Hide resolved
Copy link
Contributor

@tomlqc tomlqc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@multiphaseCFD multiphaseCFD removed the request for review from AmintorDusko September 19, 2024 14:00
Copy link
Contributor

@Qottmann Qottmann 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 the update @multiphaseCFD 👍

Still some doubts and confusion around the following points:

  • recommending to disable new opmath. This should be a very last resort as it will likely cause all kinds of unforseeable problems. If there are things not supported right now, we should just note that and aim at fixing that asap.
  • recommending to use finite shots generally. For returning things like qml.probs and getting finite samples - sure, but for expectation values? Do we really get faster performance when using finite shots? Would be very strange if that was the case 🤔

doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
multiphaseCFD and others added 3 commits September 19, 2024 11:34
Co-authored-by: Korbinian Kottmann <43949391+Qottmann@users.noreply.github.com>
Copy link
Contributor

@AmintorDusko AmintorDusko 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!

doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>
doc/lightning_tensor/device.rst Outdated Show resolved Hide resolved
doc/lightning_tensor/device.rst Show resolved Hide resolved
@multiphaseCFD multiphaseCFD merged commit f833461 into master Sep 20, 2024
75 of 76 checks passed
@multiphaseCFD multiphaseCFD deleted the update_mps_docs branch September 20, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge PR until this label is removed urgent Mark a pull request as high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants