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

Generalize seeding for measurements #1003

Merged
merged 44 commits into from
Dec 3, 2024

Conversation

rauletorresc
Copy link
Contributor

@rauletorresc rauletorresc commented Nov 22, 2024

Context: Currently, only catalyst.measure(), qml.sample() and qml.counts() can be seeded with qjit(seed=N). It would be nice to be able to seed all circuit readout methods, like probs(), expval(), var(), when executing a device with shots.

Description of the Change: Let the base measurement class store the device seed so it can be propagated when necessary to newly created derived measurements. Move the logic from the simulator to the base measurement class.

Benefits: All measurements using the generate_samples method will be seeded.

Measures to test locally:

  • expval
  • var
  • probs
  • sample
  • counts

Devices to modify

  • LQ
  • LK
  • LG

TODO

  • Simplify design
  • Document changes
  • Test probs using the LQ, LK and LG plugins
  • Test expval using the LQ, LK and LG plugins
  • Test var using the LQ, LK and LG plugins
  • Update changelog

[sc-77124]

@rauletorresc rauletorresc requested review from a team November 22, 2024 16:47
@rauletorresc rauletorresc self-assigned this Nov 22, 2024
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.65%. Comparing base (074ddcf) to head (b46a8de).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (074ddcf) and HEAD (b46a8de). Click for more details.

HEAD has 57 uploads less than BASE
Flag BASE (074ddcf) HEAD (b46a8de)
65 8
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1003      +/-   ##
==========================================
- Coverage   97.71%   91.65%   -6.06%     
==========================================
  Files         228      176      -52     
  Lines       36479    24744   -11735     
==========================================
- Hits        35645    22680   -12965     
- Misses        834     2064    +1230     

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

Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

This looks like a clean solution to me! 💯 Have you tested that the randomness is now completely fixed for all measurements (e.g. decimals do not vary in an expval etc)?

@rauletorresc
Copy link
Contributor Author

This looks like a clean solution to me! 💯 Have you tested that the randomness is now completely fixed for all measurements (e.g. decimals do not vary in an expval etc)?

Sorry to ask, but how can I test this :)

@paul0403
Copy link
Contributor

This looks like a clean solution to me! 💯 Have you tested that the randomness is now completely fixed for all measurements (e.g. decimals do not vary in an expval etc)?

Sorry to ask, but how can I test this :)

https://github.com/PennyLaneAI/catalyst/blob/main/frontend/test/pytest/test_seeded_qjit.py

What I would do is on your local branch you can build catalyst first (make all ), this installs whatever lightning specified in wherever, then you can build your own lightning branch locally so it overwrites the previous installation

@dime10
Copy link
Contributor

dime10 commented Nov 22, 2024

This looks like a clean solution to me! 💯 Have you tested that the randomness is now completely fixed for all measurements (e.g. decimals do not vary in an expval etc)?

Sorry to ask, but how can I test this :)

https://github.com/PennyLaneAI/catalyst/blob/main/frontend/test/pytest/test_seeded_qjit.py

What I would do is on your local branch you can build catalyst first (make all ), this installs whatever lightning specified in wherever, then you can build your own lightning branch locally so it overwrites the previous installation

In addition to running those tests for existing functionality, we should check that something like

@qml.qnode(qml.device("lightning.qubit", wires=1, shots=100))
def circuit():
    qml.RY(0.7, wires=0)
    return qml.expval(qml.PauliZ(0))

produces the same decimals with repeated runs if seeded.

@paul0403
Copy link
Contributor

paul0403 commented Nov 22, 2024

Acatully, I just remembered you can test this directly in lightning-catalyst, and no need to go through frontend https://github.com/PennyLaneAI/pennylane-lightning/pull/927/files#diff-934a38a9af759b117f260f29c5b2732bee701b7cd3b49a4bdf22da7f69973ad8

Edit: although adding expval/probs to the catalyst frontend tests mentioned above might also be necessary, just to make sure nothing's wrong during mlir.

@rauletorresc
Copy link
Contributor Author

@dime10 @paul0403 after the last changes, expval does produce the same decimals with repeated runs if seeded. I'll perform similar checks with the rest of the measures. Keep you posted.

@rauletorresc
Copy link
Contributor Author

@dime10 @paul0403 after the last changes, expval does produce the same decimals with repeated runs if seeded. I'll perform similar checks with the rest of the measures. Keep you posted.

Confirmed also with var, probs, sample and counts. And I learned state does not work with finite shots.

@paul0403
Copy link
Contributor

Can we have tests similar to https://github.com/PennyLaneAI/pennylane-lightning/pull/927/files#diff-934a38a9af759b117f260f29c5b2732bee701b7cd3b49a4bdf22da7f69973ad8? So tests with the lightning-catalyst interface, not just the lightning base classes.

Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Nice work @rauletorresc! Happy to review the complete version of the support

@rauletorresc rauletorresc requested review from paul0403, maliasadi and a team November 28, 2024 17:52
@paul0403
Copy link
Contributor

paul0403 commented Nov 28, 2024

Looks good! Thanks @rauletorresc for working on this! 💯

As a final sanity check, can you maybe build against the lightning in this branch, and do some catalyst frontend tests like https://github.com/PennyLaneAI/catalyst/blob/main/frontend/test/pytest/test_seeded_qjit.py?

For example,

    dev = qml.device("lightning.qubit", wires=2, shots=100)

    @qjit(seed=37)
    def workflow():
        @qml.qnode(dev)
        def circuit():
            qml.Hadamard(wires=[0])
            qml.RX(12.34, wires=[1])
            return qml.probs()

        return circuit(), circuit(), circuit(), circuit()

   trail1 = workflow()  # [probs10, probs11, probs12, probs13]
   trail2 = workflow()  # [probs20, probs21, probs22, probs23]

   for (probs1i, probs2i) in zip(trail1, trail2):
      assert probs1i == probs2i // or whatever the equivalent pytest syntax is

Note that within the same workflow() call the four circuit() calls should give different results, i.e. the four probs1* and the four probs2* should be different among themselves.

(This should also be a PR in catalyst, after this lightning PR is merged)

@maliasadi maliasadi changed the title [WIP] Generalize seeding for measurements Generalize seeding for measurements Nov 28, 2024
@rauletorresc
Copy link
Contributor Author

Looks good! Thanks @rauletorresc for working on this! 💯

As a final sanity check, can you maybe build against the lightning in this branch, and do some catalyst frontend tests like https://github.com/PennyLaneAI/catalyst/blob/main/frontend/test/pytest/test_seeded_qjit.py?

For example,

    dev = qml.device("lightning.qubit", wires=2, shots=100)

    @qjit(seed=37)
    def workflow():
        @qml.qnode(dev)
        def circuit():
            qml.Hadamard(wires=[0])
            qml.RX(12.34, wires=[1])
            return qml.probs()

        return circuit(), circuit(), circuit(), circuit()

   trail1 = workflow()  # [probs10, probs11, probs12, probs13]
   trail2 = workflow()  # [probs20, probs21, probs22, probs23]

   for (probs1i, probs2i) in zip(trail1, trail2):
      assert probs1i == probs2i // or whatever the equivalent pytest syntax is

Note that within the same workflow() call the four circuit() calls should give different results, i.e. the four probs1* and the four probs2* should be different among themselves.

(This should also be a PR in catalyst, after this lightning PR is merged)

Something like that I am doing at the Catalyst side here: PennyLaneAI/catalyst#1344
It works locally, I hope it will work in the CI once this PR gets merged.

Copy link
Contributor

@paul0403 paul0403 left a comment

Choose a reason for hiding this comment

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

💯 🥳

All good from my side! I'm unfamiliar with the lightning codecov so maybe someone from performance can suggest there.

Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Nice work @rauletorresc! Happy to approve 🥳

@rauletorresc rauletorresc merged commit 3cdd7e4 into master Dec 3, 2024
114 of 115 checks passed
@rauletorresc rauletorresc deleted the raultorres/generalize_measures_seeding branch December 3, 2024 02:13
rauletorresc added a commit to PennyLaneAI/catalyst that referenced this pull request Dec 3, 2024
**Context:** Once
PennyLaneAI/pennylane-lightning#1003 gets merged
into Lightning, we can test `expval`, `var` and `probs` with a seed from
the Catalyst frontend.

**Description of the Change:** Add the corresponding tests.

[sc-77124]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:build_wheels Activate wheel building.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants