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

None is not a valid wire label #6797

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

None is not a valid wire label #6797

wants to merge 17 commits into from

Conversation

mudit2812
Copy link
Contributor

Follow up to #6713 . Users can still provide None as a wire label if it is inside a sequence. This PR removes the ability to do so.

PietropaoloFrisoni and others added 9 commits January 6, 2025 09:23
**Context:**

We weren't actually validating that the `device_vjp=True` was supported.
This allowed strange circumstances where you would have
`device_vjp=True`, but not support device derivatives at all, and this
could lead to incorrect answers.

**Description of the Change:**

**Benefits:**

**Possible Drawbacks:**

**Related GitHub Issues:**
**Context:**

When running tests locally, I was getting failures like:
```
FAILED tests/devices/default_qubit/test_default_qubit.py::TestPostselection::test_postselection_invalid_finite_shots[shots1-mp2-expected_shape2-False-jax] - ValueError: Probabilities contain NaN
```

We had logic to catch the error `'probabilities contain NaN'`, but
something about my versioning gives a slightly different capitalization.
To prevent this happening of other people, I updated the error catching
to be capitalization independent.

**Description of the Change:**

Catch both `"Probabilities contain NaN"` and `"probabilities contain
NaN"`.

**Benefits:**

Tests pass locally for my environment.

**Possible Drawbacks:**

Still a bit error prone. I'm also not sure what about my environment
gives different capitalization.

**Related GitHub Issues:**

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Pietropaolo Frisoni <pietropaolo.frisoni@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: ringo-but-quantum <github-ringo-but-quantum@xanadu.ai>
Co-authored-by: Andrija Paurevic <46359773+andrijapau@users.noreply.github.com>
**Context:**

PR #6019 only fixes `BasisRotation` when using backprop on
`default.qubit`. It is not jit compatible on any other device. This is
because `unitary_matrix` was being considered a hyperparameter, not a
piece of data. So we could not detect that the matrix was a tracer and
we were in jitting mode, and we could not convert the matrix back into
numpy data.

**Description of the Change:**

Make `unitary_matrix` a piece of data instead of a hyperparameter. This
allows us to detect when it is being jitted.

As a by-product, I also made it valid pytree.

By making `unitary_matrix` a piece of data, we were able to get rid of
the custom comparison method in `qml.equal`.

**Benefits:**

**Possible Drawbacks:**

**Related GitHub Issues:**

[sc-51603] Fixes #6004
**Context:**

Source-code improvements found during the v0.40 quality assurance
process.

Documentation focused improvements are addressed in #6774.

**Description of the Change:**

A few things were adjusted in this PR,

**Bosonic Operators**

- `BoseX` and `FermiX` are now both top-level (as suggested by product).
- `BoseX` now raises a `TypeError` (instead of a `ValueError` for an
inappropriate operator type (was suggested in the original PR but was
never implemented).
- Minor documentation clean-up due to top-level access adjustment.
Remake of #6791
Context:
Had subprocess.Popen in the original code and should be removed for
security purposes

Description of the Change:
Changed to use Python standard lib

Benefits:
Security
Update logic in `LegacyDevice` to check for whether the device supports
`LinearCombination` or not.
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.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 Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (cc6849a) to head (5699440).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6797   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files         476      476           
  Lines       45222    45222           
=======================================
  Hits        45045    45045           
  Misses        177      177           

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

@PietropaoloFrisoni PietropaoloFrisoni added this to the v0.40 milestone Jan 9, 2025
@PietropaoloFrisoni
Copy link
Contributor

Thanks @mudit2812, I added this to the milestone since it is targeting the rc branch. Please let me know if the plan changed and this should be part of PL 0.41

@mudit2812
Copy link
Contributor Author

Sorry about that! I forgot to add it to the mileatone

@mudit2812
Copy link
Contributor Author

I'd also like opinions on whether this change makes sense or not. The check may get expensive for large wire registers

Copy link
Contributor

@JerryChen97 JerryChen97 left a comment

Choose a reason for hiding this comment

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

LGTM

@JerryChen97
Copy link
Contributor

@mudit2812 mudit2812 removed this from the v0.40 milestone Jan 10, 2025
@mudit2812
Copy link
Contributor Author

For the sake of thoroughness, I'm removing this from the v0.40 milestone. It is not a particularly pressing change.

@mudit2812 mudit2812 changed the base branch from v0.40.0-rc0 to master January 10, 2025 18:24
@mudit2812 mudit2812 added the do not merge ⚠️ Do not merge the pull request until this label is removed label Jan 10, 2025
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 the pull request until this label is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants