Skip to content

Introduction of ExperimentalWarning#11326

Merged
1ucian0 merged 6 commits into
Qiskit:mainfrom
1ucian0:experimentalwarning/1
Jan 31, 2024
Merged

Introduction of ExperimentalWarning#11326
1ucian0 merged 6 commits into
Qiskit:mainfrom
1ucian0:experimentalwarning/1

Conversation

@1ucian0
Copy link
Copy Markdown
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
Copy Markdown
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

@1ucian0 1ucian0 force-pushed the experimentalwarning/1 branch from c586aa9 to a66d35b Compare November 27, 2023 15:56
@coveralls
Copy link
Copy Markdown

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
Copy Markdown
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.

Comment thread qiskit/utils/experimental.py Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread qiskit/exceptions.py
jakelishman and others added 2 commits January 31, 2024 08:58
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
@jakelishman jakelishman enabled auto-merge January 31, 2024 10:38
@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
@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.

5 participants