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

Introduction of ExperimentalWarning #11326

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Nov 27, 2023

The stability policy introduces the support for experimental APIs. The documentation says that an experimental API should have a warning in the documentation and raise a UserWarning. Here is a suggestion for that second part. For the first one, decorators like in the deprecation case are probably prefer.

@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @enavarro51
  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia
  • @ikkoham
  • @levbishop
  • @mtreinish
  • @nkanazawa1989
  • @t-imamichi

@coveralls
Copy link

coveralls commented Nov 27, 2023

Pull Request Test Coverage Report for Build 7724747077

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 149 unchanged lines in 17 files lost coverage.
  • Overall coverage increased (+0.1%) to 89.587%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.81%
qiskit/circuit/parametervector.py 1 93.33%
qiskit/providers/backend.py 2 79.73%
qiskit/providers/models/backendstatus.py 2 74.07%
qiskit/transpiler/passes/basis/basis_translator.py 2 97.64%
qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py 3 97.25%
crates/qasm2/src/lex.rs 4 91.94%
qiskit/qpy/common.py 4 91.14%
qiskit/compiler/transpiler.py 7 92.96%
qiskit/qpy/interface.py 8 89.33%
Totals Coverage Status
Change from base Build 7709087344: 0.1%
Covered Lines: 59516
Relevant Lines: 66434

💛 - Coveralls

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @1ucian0 I like new warning class, since general UserWarning is hard to search for and could be left unintentionally. I guess it's also convenient to have some unit test util or base class that automatically ignore the warning because it's bit cumbersome to write with self.assertWarns(ExperimentalQiskitAPI): for every test case for the new feature.

def __init__(self, api_name):
self.api_name = api_name
super().__init__(
f"Calling {self.api_name} is experimental and it might be changed or removed at "
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we emit this warning in a constructor of some class (if the entire class is experimental)? Saying Calling MyNewClass.__init__() is experimental... seems bit awkward.

@1ucian0 1ucian0 added this to the 1.0.0 milestone Dec 4, 2023
@jakelishman
Copy link
Member

I'd forgotten that this existed, and I've made #11522 that similarly adds a QiskitWarning base class. I think qiskit.exceptions is the correct place for the warnings (Warning is a subclass of BaseException in Python), and that PR also moves QPYLoadingDeprecatedFeatureWarning to be a QiskitWarning base, and adds documentation, etc.

I think #11522 is close to merge - maybe best to retarget this PR on top of that once it's in, and use the to-be-existing QiskitWarning?

@jakelishman
Copy link
Member

Luciano: are you interested in getting this rebased over main and getting it in? I'd suggest putting ExperimentalWarning in qiskit.exceptions, which is where QiskitWarning is now already defined on main (see #11522).

I'm thinking that a sensible story for integrating the new OQ3 importer could be to expose the native one as qiskit.qasm3.loads_experimental or similar, so it's immediately available, and switch it in for qiskit.qasm3.loads once it's complete.

@jakelishman
Copy link
Member

I've got an implementation of this and some short-hand documentation in #11584 that you might want to pinch - I just stuck it in #11584 to demo how I'd use it in an interface, but it should merge in this PR, not that one.

@jakelishman
Copy link
Member

In the interests of moving this PR forwards, I've switched out the previous implementation for the version that I had previously included in #11584, which builds on top of the QiskitWarning that appeared in #11522.

I've not included a particular error message in the warning - I'm not fully convinced there's value added by it, when the warning class name and documentation of the class describe it. I've also not included any decorators to apply it - imo it should be very infrequently used and only in very specialised situations, where it'll be most appropriate for people to write their own specific message explaining the situation.

Let me know what you think on it - I've left the previous commit as an object in this PR, even though I reverted it with a5a5605, so the PR can easily be rolled back to it if necessary.

jakelishman and others added 2 commits January 31, 2024 08:58
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
@jakelishman jakelishman added this pull request to the merge queue Jan 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2024
@1ucian0 1ucian0 added this pull request to the merge queue Jan 31, 2024
Merged via the queue into Qiskit:main with commit f1fa4f2 Jan 31, 2024
12 checks passed
@1ucian0 1ucian0 deleted the experimentalwarning/1 branch January 31, 2024 12:53
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.

None yet

5 participants