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 deprecation warning to from_qasm #5882

Merged
merged 9 commits into from
Jun 20, 2024
Merged

Add deprecation warning to from_qasm #5882

merged 9 commits into from
Jun 20, 2024

Conversation

astralcai
Copy link
Contributor

Context:
Previously, the QASM circuit converted to PL had no measurements (even if the circuit ended with measurements, the assumption was we remove those and add PL measurements once the circuit is converted).

In the UnitaryHack, a contributor added the option to specify measurements when converting a circuit from_qasm, like the from_qiskit function. In doing so, the from_qasm circuit took on the current from_qiskit convention. Calling from_qasm(qasm_str) previously returned only the operators, and now returns operators + measurements. This is a breaking change with no deprecation cycle.

PennyLaneAI/pennylane-qiskit#469

Description of the Change:
Adds a deprecation warning to from_qasm

[sc-66318]

Copy link
Contributor

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 Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (14a0b63) to head (71e7d33).
Report is 243 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5882      +/-   ##
==========================================
- Coverage   99.67%   99.66%   -0.01%     
==========================================
  Files         422      422              
  Lines       40658    40367     -291     
==========================================
- Hits        40525    40233     -292     
- Misses        133      134       +1     

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

@astralcai astralcai added this to the v0.37 milestone Jun 20, 2024
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, thanks! 🚀

astralcai and others added 2 commits June 20, 2024 14:14
Co-authored-by: lillian542 <38584660+lillian542@users.noreply.github.com>
pennylane/io.py Outdated Show resolved Hide resolved
pennylane/io.py Outdated Show resolved Hide resolved
Co-authored-by: Christina Lee <christina@xanadu.ai>
Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

LGTM! Just two small suggestions.

pennylane/io.py Outdated Show resolved Hide resolved
pennylane/io.py Outdated Show resolved Hide resolved
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
@timmysilv
Copy link
Contributor

don't forget to add an entry in deprecations.rst!

@astralcai
Copy link
Contributor Author

don't forget to add an entry in deprecations.rst!

ooops, will do.

@astralcai astralcai enabled auto-merge (squash) June 20, 2024 20:57
@astralcai astralcai merged commit 50e009a into master Jun 20, 2024
40 checks passed
@astralcai astralcai deleted the qasm-dep branch June 20, 2024 21:40
Comment on lines +507 to +515
if measurements is False:
warnings.warn(
"The current default behaviour of removing measurements in the QASM code is "
"deprecated. Set measurements=None to keep the existing measurements in the QASM "
"code or set measurements=[] to remove them from the returned circuit. Starting "
"in version 0.38, measurements=None will be the new default.",
qml.PennyLaneDeprecationWarning,
)
measurements = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I was expecting to only raise a warning if measurements is False and there are measurements present in the QASM circuit. Is that something we could determine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make another PR to do that.

astralcai added a commit that referenced this pull request Jun 28, 2024
#5904)

**Context:**
#5882 led to deprecation
warnings raised in non-applicable scenarios

**Description of the Change:**
Only raise deprecation warning if the circuit contains measurements

**Benefits:**
Less confusing deprecation warnings
mudit2812 pushed a commit that referenced this pull request Jul 2, 2024
**Context:**
Previously, the QASM circuit converted to PL had no measurements (even
if the circuit ended with measurements, the assumption was we remove
those and add PL measurements once the circuit is converted).

In the UnitaryHack, a contributor added the option to specify
measurements when converting a circuit `from_qasm`, like the
`from_qiskit` function. In doing so, the `from_qasm` circuit took on the
current `from_qiskit` convention. Calling `from_qasm(qasm_str)`
previously returned only the operators, and now returns operators +
measurements. This is a breaking change with no deprecation cycle.

PennyLaneAI/pennylane-qiskit#469

**Description of the Change:**
Adds a deprecation warning to `from_qasm`

[sc-66318]

---------

Co-authored-by: lillian542 <38584660+lillian542@users.noreply.github.com>
Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
mudit2812 pushed a commit that referenced this pull request Jul 2, 2024
#5904)

**Context:**
#5882 led to deprecation
warnings raised in non-applicable scenarios

**Description of the Change:**
Only raise deprecation warning if the circuit contains measurements

**Benefits:**
Less confusing deprecation warnings
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.

6 participants