Have IdentityOperator inherit from UnitaryOperator and HermitianOperator instead of Operator

[vtomole]

Brief description of what is fixed or changed

The Identity operator is unitary. This change is to reflect that.

Release Notes

• physics.quantum
  • Change the baseclass of IdentityOperator from Operator to HermitianOperator and UnitaryOperator

[sympy-bot]

✅

Hi, I am the SymPy bot. 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
  • Change the baseclass of IdentityOperator from Operator to HermitianOperator and UnitaryOperator (#24910 by @vtomole)

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

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

#### Brief description of what is fixed or changed
The [Identity operator is unitary](https://en.wikipedia.org/wiki/Identity_matrix). This change is to reflect that.

#### 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:




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
  * Change the baseclass of `IdentityOperator` from `Operator` to `HermitianOperator` and `UnitaryOperator`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[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
     [8bd31f66]       [aa3417d2]
     <sympy-1.12rc1^0>                 
-        81.9±3ms       53.3±0.4ms     0.65  integrate.TimeIntegrationRisch02.time_doit_risch(10)

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

[sylee957]

What about if IdentityOperator have to inherit from HermitianOperator?

[vtomole]

Oh, right. Identity is hermitian as well. I've updated to PR to have IdentityOperator inherit from both.

[sylee957]

It could be better to avoid subtyping relationship to model intersection properties like Identity, because it would necessarily involve multiple inheritance, which is often the antipattern choice.

[vtomole]

What about if IdentityOperator have to inherit from HermitianOperator?

I might have misunderstood this question. In what scenario would IdentityOperator have to inherit from HermitianOperator?

[sylee957]

In what scenario would IdentityOperator have to inherit from HermitianOperator?

Then I would also raise an obvious question why would you want to change the design to achieve that.
I don't think that it fixes some nontrivial bug than changing the definitions.

[Costor]

I see vtomole's point - its about consistency of properties. If you write some more generic code and use classes as indicator of properties it is strange that you have to write code like If instance(A, UnitaryOperator) or isinstance(A, IdentityOperator) to check if A is unitary. Same goes with hermitian.

But SymPy is actually not trying to use classes for that purpose (probably because of technical issues, and because more flexibility is needed, see below). As far as I can see it uses .is_xxxx properties instead, or more generally put the SymPy attribute system! So see HermitianOperator: it defines the .is_hermitian property. That could be a start:

I would rather vote for introducing the attributes .is_hermitian and .is_unitary into Operator. They would inherit to HermitianOperator, UnitaryOperator, and IdentityOperator, also OuterProduct etc. and be set there correctly. From UnitaryOperator they would be inherited to Gate, and .is_hermitian could be set to True for those gate classes that are both unitary and hermitian (as Hadamard and Pauli gates, and not to forget the IdentityGate :-) ). This is also a good example for flexibility - making these gates also inherit from HermitianOperator would become cumbersome.

[sylee957]

I also had in mind that other logical programing is better than inheritance though.
If the subtyping test is used for testing some property of an object, the argument about uniqueness is needed, to avoid complicated diamond inheritance problems.

[vtomole]

.is_hermitian could be set to True for those gate classes that are both unitary and hermitian

Fact check me on this: I'm pretty sure that every unitary matrix is hermitian. If that's true, there is no need for is_hermitian = True in the Gate classes. We can set it once in UnitaryOperator.

[sylee957]

I'm pretty sure that every unitary matrix is hermitian.

I'm sure that they are mathematically unrelated, and counterexamples are easily found

• Matrix([[1, I], [-I, 1]]) / 2 Hermitian, but not Unitary
• Matrix([[1, I], [I, 1]]) / sqrt(2) Unitary but not Hermitian

[vtomole]

I've added is_hermitian and is_unitary to Operator as suggested by @Costor

[Costor]

In the current form of the PR the new attributes .is_hermitian and .is_unitary are set in HermitianOperator and UnitaryOperator only. In all other classes that inherit from Operator the attributes are left unmodified which might be confusing if people rely on them. For instance all gates are unitary, the Pauli Gates X, Y, Z are even unitary and hermitian. In OuterProduct it is hermitian in case bra == ket. Is there a chance to add this?

[vtomole]

I've set the is_hermitian for IdentityGate, T, S and Swap.

In OuterProduct it is hermitian in case bra == ket. Is there a chance to add this?

Unfortunately we can't access self in a class variable (is_hermitian = self.bra == Dagger(self.ket)). Maybe there is another way.

[Costor]

T and S are not (in general) hermitian, so require .is_hermitian=False.
Swap is hermitian, so ok.
The three Pauli gates XGate, YGate, ZGate and the Hadamard gate all are hermitian, so should get .is_hermitian=True .

CNot is hermitian, but inherits from HermitianOperater already, so nothing to be done there.

[vtomole]

T and S are not (in general) hermitian, so require .is_hermitian=False.

This has already been done in this PR

The three Pauli gates XGate, YGate, ZGate and the Hadamard gate all are hermitian, so should get .is_hermitian=True .

This is not neccessary because all these gates inherit from HermitianOperator

CNot is hermitian, but inherits from HermitianOperater already, so nothing to be done there.

👍

[Costor]

It seems all questions have been answered a month ago.
What keeps this PR from being merged into master?