[Testing] Fix types in operations for dense pauli strings

[alexbouayad]

Fix #4835.

Fix operations like __pow__, __mul__, __rmul__, __abs__, tensor_product, etc, on abstract class BaseDensePauliString so that the operations return compatible types when applied on the two concrete subclasses DensePauliString and MutableDensePauliString.

Also add a few type annotations, and modify a few tests.

One noticeable implied change: multiplying a MutableDensePauliString object with a DensePauliString object (and vise versa) is not allowed anymore.

[tanujkhattar]

Why the type ignore? (here and elsewhere). Should we revert to not having type annotations in these cases instead of explicitly ignoring them?

[alexbouayad]

The thing here is that BaseDensePauliString is a subclass of raw_types.Gate. As a result, mypy expects BaseDensePauliString.__rmul__ to return an object of a subclass of cirq.LinearCombinationOfGates. That is not the case, as the operation actually acts on the coefficient attribute of the Pauli string. Maybe a natural fix would be for the operations in raw_types.Gate to have Union[Gate, cirq.LinearCombinationOfGates] as return type instead of cirq.LinearCombinationOfGates alone. I'll commit the fix for review.

[alexbouayad]

I rollbacked the previous change, that is, the return type of linear operations in raw_types.py is now back to cirq.LinearCombinationOfGates alone. I used Gate's method wrap_in_linear_combination to convert the results of linear operations in dense_pauli_string.py (I mimicked what's done in raw_types.py).

[tanujkhattar]

Left another round of comments. Main concern is about potential breaking changes when doing binary operations on dense pauli strings of different type types.

[tanujkhattar]

This should be type(self).eye since eye is a class method. We should also have a test to verify this works as expected.

Suggested change

	return coef * self.eye(len(self))
	return coef * type(self).eye(len(self))

[alexbouayad]

• self.eye and type(self).eye are semantically equivalent here. Is your suggestion referring to some convention or best practice?
• I'll add the test.

[alexbouayad]

I changed to type(self) as suggested.

[tanujkhattar]

Does this mean multiplying a MutableDensePauliString instance with a DensePauliString was allowed before this change but will not be allowed anymore? This sounds like a breaking change and should not be done.

For binary operations that expect an other argument which can be a base dense pauli string, I think we should default to returning a MutableDensePauliString if the types of self and other are not same (but both are instances of BaseDensePauliString), else return type(self).

This should be done for __mul__, __rmul__, tensor_product etc. Please also add corresponding tests.

[alexbouayad]

I added the possibility to cross operate on mutable and non-mutable dense Pauli strings.

[vtomole]

Friendly ping @alexandrebouayad: Do you have time to finish this up? I can take it on if not.

[alexbouayad]

Friendly ping @alexandrebouayad: Do you have time to finish this up? I can take it on if not.

@vtomole thanks for the reminder, it had slipped my mind. I'll do it in the coming days.

[alexbouayad]

I have made the changes as suggested, plus a few other minor ones. I'll add the remaining tests, and I'll ensure that local checks pass by the end of this week hopefully.

[tanujkhattar]

@alexandrebouayad Can you please fix the remaining failing tests? Thanks!

[tanujkhattar]

@alexandrebouayad Gentle reminder, do you still have time to work on this?

[vtomole]

Hey @alexandrebouayad , I'm pretty sure this PR has been superseded by #5624. We would love your review on that PR.

[tanujkhattar]

@vtomole Actually, I did skip over some of the tricky parts of fixing mypy types in my PR #5624; for example:

Adding return types annotations to operations like __neg__, __add__ etc. breaks mypy because the cirq.Gate class returns LinearCombinationOfGates by default for these methods, but in BaseDensePauliString, we want to return type(self)(...), which is a (subclass of) BaseDensePauliString and not a LinearCombinationOfGates. This breaks Liskov Substitution Principle because BaseDensePauliString is not a subclass of LinearCombinationOfGates.

Fixing this would require us to modify the default return types on cirq.Gate, which will be a big breaking change. We can potentially open a new issue to track this and maybe @alexandrebouayad's PR can tackle the new issue if he's already thought on this (I see the solution he has applied in his PR is to completely remove the return types from cirq.Gate's methods -- I'm not sure if this is the best we can do though; it's probably better to not have return types on a specific implementation like BaseDensePauliString instead of removing return types from cirq.Gate)

[alexbouayad]

@vtomole Thanks for letting me now, I'll try and have a look at #5624.

@tanujkhattar I have managed to add full return type annotations for both cirq.Gate and BaseDensePauliString (plus DensePauliString and MutableDensePauliString). Mypy seems happy with it.

For the latter to work, I had to remove NotImplementedType from the return types of the binary special methods, as the workaround used in Cirq equates NotImplementedType with Any (leading to theoretical and practical typing inconsistencies). This seems to me to be the natural (or at least a more natural) thing to do, as NotImplemented is actually never returned by (public) operators based on corresponding (private) binary special methods; see https://docs.python.org/3/library/constants.html#NotImplemented.