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

Add rng and prng_key to get_final_state, apply_operation #5337

Merged
merged 27 commits into from
Apr 26, 2024
Merged

Conversation

mudit2812
Copy link
Contributor

@mudit2812 mudit2812 commented Mar 7, 2024

Context:
The seed provided to DefaultQubit during initialization is only used for measurements. However, we now have uses for it while applying operations, specifically with applying mid-circuit measurements with dynamic_one_shot, and updating the number of shots with postselection with defer_measurements. This PR updates devices.qubit.simulate.get_final_state and devices.qubit.apply_operation to use the RNG/PRNGKey provided during device initialization for stochastic behaviour.

Description of the Change:

  • Add rng and prng_key arguments to get_final_state and the MidMeasureMP dispatcher of apply_operation. appy_operation now uses the rng/PRNGKey to sample the measurement outcome.
  • Add rng and prng_key arguments to _postselection_postprocess. This function is used for renormalizing the state after a Projector is applied to it, as well as to update the number of shots if provided. This function now uses the rng/PRNGKey to update shots using the binomial distribution.
  • Added/updated tests. The native MCM tests for default.qubit now set a seed on the device. With this change, I also removed flaky from all the native MCM tests and instead increased the number of shots for the tests that were still failing.
  • Updated get_final_state and apply_operation to accept **execution_kwargs arguments, which hide rng, prng_key, interface, and mid_measurements. All of these arguments impact execution outcomes, hence why they were collected. This also means that all of those arguments are now strictly keyword arguments and can no longer be passed to the functions as positional arguments.

Benefits:

  • Random behaviour is more consistent and expected.
  • Less random, unrelated failures in CI.

Possible Drawbacks:

Related GitHub Issues:

@mudit2812
Copy link
Contributor Author

[sc-58258]

@vincentmr vincentmr mentioned this pull request Apr 15, 2024
5 tasks
vincentmr added a commit that referenced this pull request Apr 21, 2024
### Before submitting

Please complete the following checklist when submitting a PR:

- [x] 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
      test directory!

- [x] 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`.

- [x] Ensure that the test suite passes, by running `make test`.

- [x] Add a new entry to the `doc/releases/changelog-dev.md` file,
summarizing the
      change, and including a link back to the PR.

- [x] The PennyLane source code conforms to
      [PEP8 standards](https://www.python.org/dev/peps/pep-0008/).
We check all of our code against [Pylint](https://www.pylint.org/).
      To lint modified files, simply `pip install pylint`, and then
      run `pylint pennylane/path/to/file.py`.

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


------------------------------------------------------------------------------------------------------------

**Context:**
JAX RNG keys should not be reused (in production) according to the JAX
documentation. The current usage only split the keys at the lowest level
(`_sample_state_jax`). This could cause unwanted correlations. For
example, we reuse the device key for each tape in a batch of tapes in
`default.qubit`.
 
**Description of the Change:**
Add functions and methods to split RNG keys. Split RNG keys to avoid
reusing keys.

**Benefits:**

**Possible Drawbacks:**

**Related GitHub Issues:**
#5337 

[sc-61435]
@mudit2812
Copy link
Contributor Author

Just testing out updating the durations file again. Before updating, this CI run had the following core test durations:
18, 30, 23, 24, 9 minutes.

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.69%. Comparing base (9151477) to head (46924ed).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5337      +/-   ##
==========================================
- Coverage   99.69%   99.69%   -0.01%     
==========================================
  Files         410      412       +2     
  Lines       38278    38188      -90     
==========================================
- Hits        38161    38070      -91     
- Misses        117      118       +1     

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

@mudit2812
Copy link
Contributor Author

The new timings are: 11, 22, 21, 21, 24 minutes.

@mudit2812 mudit2812 marked this pull request as ready for review April 25, 2024 02:45
@mudit2812 mudit2812 added this to the v0.36 milestone Apr 25, 2024
@albi3ro albi3ro self-requested a review April 25, 2024 15:15
@trbromley trbromley requested review from lillian542 and removed request for albi3ro April 25, 2024 15:15
@mudit2812 mudit2812 requested a review from albi3ro April 25, 2024 16:26
Copy link
Contributor

@lillian542 lillian542 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 to me 🚀 Just a couple of quick comments/questions!

pennylane/devices/qubit/apply_operation.py Show resolved Hide resolved
tests/devices/default_qubit/test_default_qubit.py Outdated Show resolved Hide resolved
@mudit2812 mudit2812 added the merge-ready ✔️ All tests pass and the PR is ready to be merged. label Apr 26, 2024
@astralcai astralcai enabled auto-merge (squash) April 26, 2024 20:03
@astralcai astralcai merged commit e82fab1 into master Apr 26, 2024
38 checks passed
@astralcai astralcai deleted the simulate-rng branch April 26, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-ready ✔️ All tests pass and the PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants