Adding a type hint to the process method of the BasePlugin class

[mo-philrelton]

This will allow other child classes to type-hint without their type-checker demanding that they return None from process. The type hint is set to Any as there are multiple process methods that return different types.

Description

Testing:

• Ran tests and they passed OK
• Added new tests for the new feature(s) - N/A

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.09%. Comparing base (84a8944) to head (fc78c7f).
⚠️ Report is 132 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2224      +/-   ##
==========================================
- Coverage   98.39%   95.09%   -3.31%     
==========================================
  Files         124      147      +23     
  Lines       12212    14853    +2641     
==========================================
+ Hits        12016    14124    +2108     
- Misses        196      729     +533     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MoseleyS]

To review this, I did a bit of googling and came across the Liskov Substitution Principle. This describes the problem that you're trying to fix - child classes of BasePlugin should override def process with a method that accepts and returns objects of the same type.

This abstract base class was written before type-hinting was adopted in IMPROVER and the default behaviour is that inputs are Any and outputs are None unless stated otherwise, which is why this PR is useful, we do NOT expect the outputs to be None. We could be more specific than Any, because we expect all IMPROVER plugins to return Union[Cube, List[Cube]], I think.

I also think that the __call__ method above should match whatever goes on the process output, because it returns whatever process does.

I'll ask for a second opinion.

[mo-philrelton]

We could be more specific than Any, because we expect all IMPROVER plugins to return Union[Cube, List[Cube]], I think.

There are a few examples of it returning other things, I've included these here:

./improver/calibration/rainforest_calibration.py:    def process(self) -> None:
./improver/nowcasting/optical_flow.py:    def process(self, cube1: Cube, cube2: Cube, boxsize: int = 30) -> Tuple[Cube, Cube]:
./improver/utilities/spatial.py:    def process(self, cube: Cube) -> Tuple[Cube, Cube]:
./improver/utilities/spatial.py:    def process(self, cube: Cube) -> Tuple[Cube, Cube]:
./improver/utilities/generalized_additive_models.py:    def process(self, gam, predictors: np.ndarray) -> np.ndarray:
./improver/wind_calculations/wind_components.py:    def process(self, wind_speed: Cube, wind_dir: Cube) -> Tuple[Cube, Cube]:
./improver/wind_calculations/wind_downscaling.py:    def process(self) -> ndarray:

[bayliffe]

I agree with Stephen that the __call__ method could have the same return type. I agree with Phil that we cannot limit this to cubes and lists of cubes now we are returning e.g. numpy arrays from PyGam. -> Any seems fine; it includes None and I could find no solid guidance on whether there was any benefit / difference using an Optional[Any] approach.

[MoseleyS]

I agree that we need to allow Tuple[Cube], I think that the examples that return arrays shouldn't be instances of BasePlugin - they violate my expectation of what a plugin is. Of course, if could be me that is wrong...

While we're at it, the same __init__.py file contains the PostProcessingPlugin ABC. Can you change the __call__ to return Any there too. I also note that this class requires the result of process to be a Cube, anything else will raise an exception. Should it have a dummy def process(...) -> Cube:?

[mo-philrelton]

I agree that we need to allow Tuple[Cube], I think that the examples that return arrays shouldn't be instances of BasePlugin - they violate my expectation of what a plugin is. Of course, if could be me that is wrong...

We could specify all the desired types in a Union[] type hint and then create issues for each of the other ones. However, this starts to fall outside the realm of my small EPP based bugbear and into the world of IMPROVER maintenance.

While we're at it, the same __init__.py file contains the PostProcessingPlugin ABC. Can you change the __call__ to return Any there too. I also note that this class requires the result of process to be a Cube, anything else will raise an exception. Should it have a dummy def process(...) -> Cube:?

I can also do this, however it would likely slow down the entire package somewhat as we would have to do an from iris.cube import Cube at the top of the file, which would be run for the entire package. This would also cause us to fall foul of the test improver_tests/cli/test_init.py::test_import_cli, which asks for large imports not to be imported.*

*EDIT: I should add, that yes we probably should do this. But it causes problems (and it doesn't affect me so much as I have yet to ever write anything importing this class) so I'm less inclined to fix it.

[MoseleyS]

... we would have to do an from iris.cube import Cube at the top of the file, which would be run for the entire package. This would also cause us to fall foul of the test improver_tests/cli/test_init.py::test_import_cli, which asks for large imports not to be imported.*

Yes, I had forgotten about that. It impacts the response time for the IMPROVER CLI if you only want the help string. Ok. Just add Any to the two __call__ methods and I'll approve it.