Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type annotations to method overrides in child classes #554

Closed
cduck opened this issue Jul 2, 2018 · 12 comments · Fixed by #5051
Closed

Add type annotations to method overrides in child classes #554

cduck opened this issue Jul 2, 2018 · 12 comments · Fixed by #5051
Assignees
Labels
area/cleancode good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/health For CI/testing/release process/refactoring/technical debt items

Comments

@cduck
Copy link
Collaborator

cduck commented Jul 2, 2018

Mypy hasn't caught that the implementation doesn't match the abstract method signature because they aren't currently type annotated.

Because of this, all gate types that implement both ParameterizableGate and TextDiagrammableGate currently have the wrong type signature for text_diagram_exponent(): text_diagram_exponent() -> Union[value.Symbol, float] vs. text_diagram_exponent() -> float.

@Strilanc Strilanc changed the title Many classes implementing abstract methods return the wrong types Add type annotations to method overrides in child classes Jul 5, 2018
@dabacon
Copy link
Collaborator

dabacon commented Apr 29, 2020

Current abstract methods that should be checked.

cirq/circuits/optimization_pass.py:100:    @abc.abstractmethod
cirq/value/abc_alt_test.py:25:        @abc.abstractmethod
cirq/value/abc_alt_test.py:44:        @abc.abstractmethod
cirq/value/abc_alt_test.py:98:        @abc.abstractmethod
cirq/value/abc_alt_test.py:177:        @abc.abstractmethod
cirq/value/abc_alt_test.py:181:        @abc.abstractmethod
cirq/contrib/acquaintance/executor.py:47:    @abc.abstractmethod
cirq/contrib/acquaintance/executor.py:55:    @abc.abstractmethod
cirq/contrib/acquaintance/executor.py:60:    @abc.abstractmethod
cirq/contrib/acquaintance/permutation.py:50:    @abc.abstractmethod
cirq/contrib/graph_device/graph_device.py:32:    @abc.abstractmethod
cirq/contrib/graph_device/graph_device.py:36:    @abc.abstractmethod
cirq/interop/quirk/cells/cell.py:48:    @abc.abstractmethod
cirq/interop/quirk/cells/cell.py:64:    @abc.abstractmethod
cirq/ops/dense_pauli_string.py:341:    @abc.abstractmethod
cirq/ops/raw_types.py:45:    @abc.abstractmethod
cirq/ops/raw_types.py:55:    @abc.abstractmethod
cirq/ops/raw_types.py:375:    @abc.abstractmethod
cirq/ops/raw_types.py:389:    @abc.abstractmethod
cirq/ops/pauli_string_raw_types.py:43:    @abc.abstractmethod
cirq/ops/arithmetic_operation.py:84:    @abc.abstractmethod
cirq/ops/arithmetic_operation.py:102:    @abc.abstractmethod
cirq/ops/arithmetic_operation.py:117:    @abc.abstractmethod
cirq/ops/eigen_gate.py:229:    @abc.abstractmethod
cirq/google/line/placement/place_strategy.py:31:    @abc.abstractmethod
cirq/google/line/placement/greedy.py:67:    @abc.abstractmethod
cirq/sim/wave_function_simulator.py:38:    @abc.abstractmethod
cirq/sim/wave_function_simulator.py:104:    @abc.abstractmethod
cirq/sim/simulator.py:93:    @abc.abstractmethod
cirq/sim/simulator.py:155:    @abc.abstractmethod
cirq/sim/simulator.py:227:    @abc.abstractmethod
cirq/sim/simulator.py:344:    @abc.abstractmethod
cirq/sim/simulator.py:404:    @abc.abstractmethod
cirq/sim/simulator.py:416:    @abc.abstractmethod
cirq/sim/wave_function.py:76:    @abc.abstractmethod
cirq/study/sweeps.py:86:    @abc.abstractmethod
cirq/study/sweeps.py:94:    @abc.abstractmethod
cirq/study/sweeps.py:98:    @abc.abstractmethod
cirq/study/sweeps.py:139:    @abc.abstractmethod
cirq/study/sweeps.py:317:    @abc.abstractmethod
cirq/study/sweeps.py:329:    @abc.abstractmethod
cirq/devices/line_qubit.py:64:    @abc.abstractmethod
cirq/devices/grid_qubit.py:69:    @abc.abstractmethod
cirq/work/sampler.py:136:    @abc.abstractmethod
cirq/work/collector.py:73:    @abc.abstractmethod
cirq/work/collector.py:93:    @abc.abstractmethod
dev_tools/check.py:52:    @abc.abstractmethod
dev_tools/check.py:56:    @abc.abstractmethod
dev_tools/check.py:60:    @abc.abstractmethod

@dabacon dabacon added the good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. label Apr 29, 2020
@balopat balopat added area/cleancode kind/health For CI/testing/release process/refactoring/technical debt items labels Sep 25, 2020
@daxfohl
Copy link
Contributor

daxfohl commented Feb 19, 2021

Fixed this for simulators in #3818. Used generics so that child classes can return more specific subtypes, and mypy will catch if they return something else. I think it's a good template for others.

@balopat
Copy link
Contributor

balopat commented Mar 17, 2021

Discussed on Cirq Cynque: heuristics for this

  • try to make the implementations follow parent's generics if there are any
  • if self type or the implementing class's type is used in any way, probably we should use generics
  • it would be great to find an automated tool to enforce child classes method type signature being compliant with the parent's

@vtomole vtomole self-assigned this Mar 17, 2021
CirqBot pushed a commit that referenced this issue Mar 24, 2021
@hohilwik
Copy link

Hi, I am interested to work on this. If this issue is still open then could you assign it to me?

@vtomole
Copy link
Collaborator

vtomole commented Aug 26, 2021

Hey @hohilwik , thanks for willing to take this on. I've assigned it to you. This will also help with #4393.

@hohilwik
Copy link

I was looking into this commit as reference for the code

90c6c99

And there are parts that I don't really understand.

- class DensityMatrixSimulator(simulator.SimulatesSamples, simulator.SimulatesIntermediateState):
+class DensityMatrixSimulator(
+   simulator.SimulatesSamples,
+    simulator.SimulatesIntermediateStateBase[
+        'DensityMatrixStepResult', 'DensityMatrixTrialResult', 'DensityMatrixSimulatorState'
+   ],
+):

Why was it required to change from simulator.SimulatesIntermediateState to the Base class? And how were the items in the square brackets decided on? This happens a lot in that commit. If someone could explain how this works that would be really helpful

@vtomole
Copy link
Collaborator

vtomole commented Aug 27, 2021

Ping @daxfohl

@daxfohl
Copy link
Contributor

daxfohl commented Aug 27, 2021

That commit added generics to the simulation classes. We could have probably gotten by more simply just by updating the return type of e.g. DensityMatrixSimulator.simulate from Iterator[StepResult] to Iterator[DensityMatrixStepResult]. But by making the simulator itself generic allows for slightly more powerful ability to write generic functions that do things with these generic classes in a type safe manner. LMK if you have any detailed questions.

@hohilwik
Copy link

Ah thanks. That does make sense to me.

@hohilwik
Copy link

Do cirq/sim/wave_function_simulator.py and cirq/sim/wave_function.py not exist anymore? I can't seem to find them

@vtomole
Copy link
Collaborator

vtomole commented Aug 29, 2021

Those were renamed to state_vector_simulator.py and state_vector.py.

@95-martin-orion
Copy link
Collaborator

Bumping for @vtomole.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cleancode good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/health For CI/testing/release process/refactoring/technical debt items
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants