Clean up types

[natestemen]

Description

A "quick" pass over all all of our # type: ignore comments, as well as typing.cast calls. Was able to remove many without any mypy error. It's a good reminder to check when refactoring code.

Things I thought about while making these changes:

1. We should have a consistent style for setting random states/seeds. Often we support int or np.random.RandomState, in some places and just a np.random.RandomState in others. Maybe we allow user facing functions to take both (maybe in addition to random.Random?), and then expect a single type once digested from the user.
2. We can definitely do better at type-hinting function arguments more generally, and return types more specifically.
3. If we want to make type changes to QPROGRAM/Executor, it's going to take some work. Fun work, however :). I played around with making QPROGRAM a typing.TypeVar, and it broke everything. A lot of things will need to change in conjuction with that, since QPROGRAM being a union means mypy's type checks are pretty loose. We'd have to be much more careful in tracking types in our conversions files.

Reviewer Note

It's probably easiest to review this commit by commit since some changes modify multiple files.

[github-actions]

👈 Launch a binder notebook on branch unitaryfund/mitiq/nts-clean-types

[codecov]

Codecov Report

Merging #1825 (e877666) into master (5c285bf) will decrease coverage by 0.03%.
Report is 1 commits behind head on master.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master    #1825      +/-   ##
==========================================
- Coverage   98.47%   98.45%   -0.03%     
==========================================
  Files          79       79              
  Lines        3681     3682       +1     
==========================================
  Hits         3625     3625              
- Misses         56       57       +1     

Files Changed	Coverage Δ
mitiq/interface/mitiq_qiskit/qiskit_utils.py	100.00% <ø> (ø)
mitiq/pec/types/types.py	99.30% <66.66%> (-0.70%)	⬇️
mitiq/benchmarks/randomized_benchmarking.py	95.00% <100.00%> (-0.24%)	⬇️
mitiq/cdr/clifford_training_data.py	98.18% <100.00%> (ø)
mitiq/executor/executor.py	98.05% <100.00%> (ø)
mitiq/interface/conversions.py	100.00% <100.00%> (ø)
mitiq/interface/mitiq_cirq/cirq_utils.py	100.00% <100.00%> (ø)
mitiq/interface/mitiq_pyquil/compiler.py	100.00% <100.00%> (ø)
mitiq/observable/observable.py	98.00% <100.00%> (ø)
mitiq/observable/pauli.py	96.21% <100.00%> (ø)
... and 5 more

[andreamari]

The reason we used random_state = np.random instead of a new instance random_state = np.RandomState() is that the user can set a global seed np.random.seed(123) with a single instruction e.g. at the beginning of a notebook and it should propagate to all Mitiq code.
Cirq uses a similar approach.

Actually I am not sure if this is good or bad practice. Just wanted to explain the motivation.

[natestemen]

Thanks for adding some background here (and the link to Cirq), it's super helpful! I agree we should definitely tread lightly around changing RandomState related things, but I'm not sure that np.random.seed works in that way. From my understanding, this function only effects the next call of a numpy random function, but not subsequent ones. So if a user sets a seed at the top of a file running Mitiq, I would not expect completely reproducible outputs.

>>> import numpy as np
>>> np.random.seed(42)
>>> np.random.randint(500)
102
>>> np.random.seed(42)
>>> np.random.randint(500)
102
>>> np.random.randint(500)
435

[andreamari]

The seed actually affects subsequent functions (the internal random states evolves in a reproducible way):

>>> import numpy as np

>>> np.random.seed(42)
>>> np.random.randint(500)
102
>>> np.random.randint(500)
435
>>> np.random.seed(42)
>>> np.random.randint(500)
102
>>> np.random.randint(500)
435

So, everything which depends on np.random becomes deterministically reproducible.

[natestemen]

Ahhh I see, thank you for clarifying. In that case, yeah I think we should avoid setting np.random.RandomState(None). I'll make the necessary changes.

[Aaron-Robertson]

I tried random_state: Union[np.random.RandomState, np.random.Generator] = np.random.default_rng(), and all tests pass. Can remove the None checks and most Optionals too, eg:

def _select(
    non_clifford_ops: Sequence[cirq.ops.Operation],
    fraction_non_clifford: float,
    method: str = "uniform",
    sigma: Optional[float] = 1.0,
    random_state: Union[np.random.RandomState, np.random.Generator] = np.random.default_rng(),
) -> List[int]:
...
    num_non_cliff = len(non_clifford_ops)
    num_to_replace = int(round(fraction_non_clifford * num_non_cliff))

Happy to push if we like it and you're low on bandwidth @natestemen.

[Aaron-Robertson]

@natestemen, as discussed made a branch to test out these changes. They're here when you get a chance to take a look!

[natestemen]

From my testing the np.random.default_rng function does not respect if the user has set a seed using np.random.seed. So I'll revert my changes here back to what we had previously.

[andreamari]

Apart from the np.random.RandomState() vs np.random problem discussed in the previous comments, all the rest LGTM

[Aaron-Robertson]

Overall great work @natestemen. Just a couple nit items (aside from the np.random convo) and otherwise LGTM

[Aaron-Robertson]

I'm getting circuits typed as any after this. I think it's good practice to do what we do in the qiskit pass above: circuit = circuit.copy()?

[natestemen]

I added the cast call back in 3af0b0f, but I think since we don't need to modify the program for a pyquil program, we do not need to copy the circuit as we do with qiskit.

[andreamari]

Nice to see so many # type ignore are gone!
Thanks Nate.