Start deprecation of complex parameters in Qiskit Pulse [2]

[TsafrirA]

Summary

This PR starts the deprecation process of complex parameters in Qiskit Pulse, by raising a PendingDeprecationWarning when one assigns a complex value to ParameterExpression in any Pulse object.

This PR replaces PR #9735 which produced inconsistent warnings (issue #9837) and was reverted.

Details and comments

PR #9002 began the deprecation of the last complex parameters in Qiskit Pulse (namely the amp parameter of pulses in the SymbolicPulse library). Complex values in ParameterExpression have caused issues in the past (for example #9187), and it seems likely that eventually complex values will be phased out. With #9002 in place, we can therefore begin heading in this direction and deprecate the use of complex parameters.

To avoid the problems identified in #9837, the caching of format_parameter_value was removed.

Sidenote

It was debated whether or not other pulse-related formatting functions (see here and here) should be merged into a single warning-yielding formatting function in the global qiskit utils (avoiding circular imports). Eventually, it was decided that this not necessary - often calibration retrieval doesn't support complex values to be begin with (see here), making the warning unnecessary and even misleading. Furthermore, the pulse utils formatting function introduces rounding, which is currently not implemented in the other formatting functions. While such rounding is important, it was decided it should not be done in this PR.

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

• @Qiskit/terra-core
• @nkanazawa1989

[coveralls]

Pull Request Test Coverage Report for Build 4596529943

• 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
• 6 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.03%) to 85.412%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/accelerate/src/sabre_swap/layer.rs	4	97.32%

Totals
Change from base Build 4588217505:	0.03%
Covered Lines:	67474
Relevant Lines:	78998

---

💛 - Coveralls

[nkanazawa1989]

Thanks for finding the correct logic to do deprecation. I think we can unify format functions after we drop this complex value support (likely conditional branch for complex value is just redundant for other inline format functions, as it is not allowed at circuit level). Let's keep logical change minimum for this PR.

[nkanazawa1989]

@Eric-Arellano Can we use your deprecation decorator for this case? Seems like predicate is good place to do type check, but writing try-except logic inside lambda function looks bit ugly.

[Eric-Arellano]

Hello! Indeed, this is a great case of using @deprecate_arg with predicate and deprecation_description.

In Python, a lambda is simply shorthand for writing a normal def function. They result in the same function object. So, here, you would create a function like this:

def _is_deprecated_operand(operand: ParameterExpression) -> bool:
   ...

Then, predicate=_is_deprecated_operand, without parantheses.

You will unfortunately need to duplicate some code because _is_deprecated_operand can only return the bool and it will not set the evaluated argument for you. So, you still need the code in this function body to set evaluated. You could get fancy and DRY that with yet another helper function -- but also this is temporary code and maybe not a big deal to duplicate

--

Here's an example of what to set deprecation_description to:

https://github.com/Qiskit/qiskit-terra/blob/6a946cf47886c3d6de4b52c98c3bacbda0d35df8/qiskit/synthesis/evolution/suzuki_trotter.py#L54-L64

[nkanazawa1989]

Yeah this is what I'm thinking of as an alternative. But writing another function for just checking type seems too much overhead, and indeed inlining the warning is much simpler. I know your decorator properly updates API docs. Does it make sense to suppress exception here?
https://github.com/Qiskit/qiskit-terra/blob/6a946cf47886c3d6de4b52c98c3bacbda0d35df8/qiskit/utils/deprecation.py#L291-L292

[nkanazawa1989]

Note that this function is not added to toctree and we don't have motivation for using decorator for the API doc update.

[Eric-Arellano]

But writing another function for just checking type seems too much overhead

What's the overhead? Code readability?

Note that this function is not added to toctree and we don't have motivation for using decorator for the API doc update.

The decorator has two benefits:

1. Update the docstring for the function so that it shows up in API docs and help(my_func) in REPLs/Jupyter
2. Standardize the message

[nkanazawa1989]

Yeah readability and maintenance overhead. We need to implement quite similar conditional branch in two separate functions. e.g.

def _is_deprecated_operand(operand, decimal):
    try:
        evaluated = complex(operand)
        evaluated = np.round(evaluated, decimals=decimal)
        return not np.isreal(evaluated)
    except TypeError:
        return False

@deprecate_arg(
   ...,
   predicate=_is_deprecated_operand,
)
def format_parameter_value(operand, decimal):
    try:
        operand = complex(operand)
        evaluated = np.round(evaluated, decimals=decimal)
        if np.isreal(evaluated):
            ...
    except TypeError:
        return operand

This doesn't seem to me like readable and efficient (because it evaluates symbolic expression twice). Also we cannot pass decimal to the predicate function because this line ignores *args.
https://github.com/Qiskit/qiskit-terra/blob/751fa46947aa1cc581b0fc25673236e396397b0f/qiskit/utils/deprecation.py#L169-L177

Again, this function is not exposed to end-users and documentation doesn't become good motivation for us, though standardization of error massage is worth considering (I'm basically a big fan of this decorator). Currently I tend to approve this PR as it is if you don't have strong objection. It would be great in future if the deprecate util could support such (edge) case more efficiently.

[Eric-Arellano]

Again, this function is not exposed to end-users and documentation doesn't become good motivation for us, though standardization of error massage is worth considering

Here's the standard format:

https://github.com/Qiskit/qiskit-terra/blob/751fa46947aa1cc581b0fc25673236e396397b0f/test/python/utils/test_deprecation.py#L152-L155

You'd want to change the first part of the first sentence to say something like

Passing a complex to the argument arg5 in the function blah is pending...

--

Currently I tend to approve this PR as it is if you don't have strong objection.

No objections from me to approve the PR as is :) The main thing I care about is docs having the deprecations, which isn't relevant here.

It would be great in future if the deprecate util could support such (edge) case more efficiently.

Hm, idea: #9902

[TsafrirA]

Thanks @Eric-Arellano for the detailed explanation!