Fix the Ket*Op -> Op*Ket Bug in qapply and _apply_operator

[Costor]

References to other Issues or PRs

Fixes #24158

Brief description of what is fixed or changed

See issue #24158. It might also fix #6073 (not tested).
qapply may wrongly associate kets to operators from the left, e.g. |1> X -> |0>, which leads to wrong results. Note in the example Qubit(1)=|1>, QubitBra(1)=<1| and XGate(0)=X the CNOT gate:

from sympy import Mul
from sympy.physics.quantum.qubit import Qubit, QubitBra
from sympy.physics.quantum.gate import XGate
from sympy.physics.quantum.qapply import qapply
P0 = Qubit(0)*XGate(0) # |0>*X is undefined
print(qapply(P0)) 
#result:  |1>  !!  qapply actually computed X*|0> !
P1 = Mul(QubitBra(0), Mul(QubitBra(0), Qubit(0)), XGate(0)) #legal expr <0| * (<1|*|1>) * X
print(qapply(P1))
#result:  0   !! expected: <0| * (<0|*|0>) * X = <0| * X = <1| = QubitBra(1)
P2 = QubitBra(0) * QubitBra(0)*Qubit(0) * XGate(0)  # 'forgot' to set brackets
print(qapply(P2))
#result:  0   !! expected: <1| as above 

You might consider P2 posed unclearly, but expression P1 is perfectly legal and may easily come up when building expressions in code.

Assessment

P0 already demonstrates the core issue: From the code analysis it seems that qapply has been written to accept and automatically 'correct' the (undefined) term Ket*Operator to Operator*Ket. However as shown above this behaviour may lead to fatally wrong results and should be removed.

Modifications

See #24158 for a technical description of the situation and the required modifications. In a nut shell:

In some Ket-classes the incorrect definitions of and calls to Ket._apply_operator(op) to compute Ket*op - which is undefined but returned op*Ket instead - are renamed to _apply_from_right_to() so qapply (which is also patched) will call them in the correct context of op*Ket only.

Other comments

._apply_operator_* and ._apply_from_right_to_* are related to each other as is .__mul__ to .__rmul__ .

Release Notes

• physics.quantum
  • In certain cases qapply would evaluate Ket * Operator in expressions as Operator * Ket. This behavior could lead to incorrect results and has been removed.

[sympy-bot]

✅

Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• physics.quantum
  • In certain cases qapply would evaluate Ket * Operator in expressions as Operator * Ket. This behavior could lead to incorrect results and has been removed. (#24164 by @Costor)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #24158

#### Brief description of what is fixed or changed
See issue #24158. It might also fix #6073 (not tested).
qapply may wrongly associate kets to operators from the left, e.g. |1> X -> |0>, which leads to wrong results. Note in the example Qubit(1)=|1>,  QubitBra(1)=<1|  and XGate(0)=X the CNOT gate:
```
from sympy import Mul
from sympy.physics.quantum.qubit import Qubit, QubitBra
from sympy.physics.quantum.gate import XGate
from sympy.physics.quantum.qapply import qapply
P0 = Qubit(0)*XGate(0) # |0>*X is undefined
print(qapply(P0)) 
#result:  |1>  !!  qapply actually computed X*|0> !
P1 = Mul(QubitBra(0), Mul(QubitBra(0), Qubit(0)), XGate(0)) #legal expr <0| * (<1|*|1>) * X
print(qapply(P1))
#result:  0   !! expected: <0| * (<0|*|0>) * X = <0| * X = <1| = QubitBra(1)
P2 = QubitBra(0) * QubitBra(0)*Qubit(0) * XGate(0)  # 'forgot' to set brackets
print(qapply(P2))
#result:  0   !! expected: <1| as above 
```
You might consider P2 posed unclearly, but expression P1 is perfectly legal and may easily come up when building expressions in code.
### Assessment ###
P0 already demonstrates the core issue: From the code analysis it seems that qapply has been written to accept and automatically 'correct' the (undefined) term `Ket*Operator` to `Operator*Ket`. However as shown above this behaviour may lead to fatally wrong results and should be removed.

### Modifications ###

See #24158 for a technical description of the situation and the required modifications. In a nut shell:

In some Ket-classes the incorrect definitions of and calls to `Ket._apply_operator(op)` to compute `Ket*op` - which is undefined but returned `op*Ket` instead - are renamed to `_apply_from_right_to()` so qapply (which is also patched) will call them in the correct context of `op*Ket` only.

#### Other comments

`._apply_operator_*` and `._apply_from_right_to_*`  are related to each other as is `.__mul__` to `.__rmul__ `.

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* physics.quantum
  * In certain cases `qapply` would evaluate  `Ket * Operator` in expressions as `Operator * Ket`. This behavior could lead to incorrect results and has been removed. 
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[oscarbenjamin]

This looks okay to me except there should be some added tests for qapply to cover the cases that were previously incorrect.

Also try to imagine someone reading the release note in the context of the release notes page:
https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11

Someone reading that won't now what "the bug" means and won't really care about internal methods like _apply_operator.

[github-actions]

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [41d90958]       [b1de2706]
     <sympy-1.11.1^0>                 
-         993±2μs          620±1μs     0.62  solve.TimeSparseSystem.time_linear_eq_to_matrix(10)
-     2.84±0.01ms         1.16±0ms     0.41  solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
-     5.63±0.01ms         1.69±0ms     0.30  solve.TimeSparseSystem.time_linear_eq_to_matrix(30)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

[Costor]

I edited the release notes to

In certain cases qapply would evaluate Ket * Operator in expressions as Operator * Ket. This behavior could lead to incorrect results and has been removed.

So some code / expressions that (maybe inadvertently) relied on this behaviour will now return correct but unexpected results. But in my oppinion we don't break an existing feature. What we do is removing an undocumented and incorrect behaviour. So no 'Breaking' remark is required.

[oscarbenjamin]

Can we say now that this fixes #6073 with the added tests?

[Costor]

It is unfortunate that no example has been given in #6073, so we cannot test it.
However after reading #6073 carefully twice again, I do no longer hope it will be fixed here. Reference to #6073 should be deleted from this fix.

It looked clearly similar, but the reason is that #6073 says that lhs and rhs don't know how to apply to each other, explicitly lhs._apply_operator(rhs) and rhs._apply_operator(lhs) both are not defined. This is definitely different from the situation I'm fixing here, where quite the opposite is the case: at least oneof lhs._apply_operator(rhs), rhs._apply_operator(lhs) are defined, but are applied incorrectly.