AL/math/MaxPooling #407
AL/math/MaxPooling #407
Conversation
There was a problem hiding this comment.
I have written some comments.
When I tried the example from the docs, I don't get the output that is written in the docs. So double check that.
And when I try to run the exact same example, but swapping numpy for torch, it doesn't work. Therefore I think that you should either make it work exactly the same for both cases, or write a clear example on what is different.
Related to that: for the torch version to work, the shapes have to be different from when running it with numpy. I can imagine that it might lead to problems when a pipeline has been created with numpy, and the user wants to run it with torch, and then gets a different result or that it doesn't work at all anymore. Maybe this is something to discuss in our next meeting.
| max function to each block. The result is a downsampled image where | ||
| each pixel value represents the maximum value within the corresponding | ||
| block of the original image. If the backend is torch, it will return the | ||
| output of `torch.nn.functional.max_pool2d` instead. |
There was a problem hiding this comment.
I don't think that it is clear from this explanation what the difference is between using the class with numpy and using it with torch. Are the first two sentences only valid in the when using numpy? Or also when using torch?
| @@ -1267,7 +1268,7 @@ class MaxPooling(Pool): | |||
| Size of the pooling kernel. | |||
| cval: number | |||
There was a problem hiding this comment.
Is "cval" used anywhere?
| automatically wrapped in an `Image` object. This behavior is handled | ||
| internally and does not affect the return type of the `get()` method. | ||
| Calling this feature returns a pooled image of the input, it will return | ||
| either numpy or torch depending on the backend. If |
There was a problem hiding this comment.
Line break a bit too early, but I don't know if that matters
| return utils.safe_call( | ||
| skimage.measure.block_reduce, | ||
| image=image, | ||
| func=self.pooling, # This will be np.mean for this class. |
There was a problem hiding this comment.
Are you sure about the np.mean?
| ): | ||
| """Method to perform max pooling with the torch backend enabled. | ||
|
|
||
| Returns the result of the image passed to a torch max |
There was a problem hiding this comment.
To early line break
| **kwargs, | ||
| ): | ||
| """Method to perform pooling with either torch or numpy backend. | ||
|
|
| else: | ||
| raise NotImplementedError(f"Backend {self.backend} not supported") | ||
|
|
||
|
|
There was a problem hiding this comment.
One line too much between the classes I think
| feature = math.MaxPooling(ksize=2) | ||
| pooled_image = feature.resolve(input_image) | ||
| self.assertTrue(np.all(pooled_image == [[6.0, 8.0]])) | ||
|
|
||
|
|
||
| # Extending the test and setting the backend to torch | ||
| @unittest.skipUnless(TORCH_AVAILABLE, "PyTorch is not installed.") |
There was a problem hiding this comment.
I can see that you haven't created the layout like this. However, it looks different from how we have done it in test_features.py for example. I personally prefer the style we use in test_feature, as all tests belonging to one class are within the same "def test_....()"
| pooled_image = feature(input_image, ksize=2) | ||
| expected = torch.tensor([[[[6.0, 8.0]]]]) | ||
| self.assertEqual(pooled_image.shape, expected.shape) | ||
| self.assertTrue(torch.allclose(pooled_image, expected)) |
There was a problem hiding this comment.
I would also add a test to check that the output is still a torch.tensor
| feature = math.MaxPooling(ksize=2) | ||
| pooled_image = feature.resolve(input_image) | ||
| self.assertTrue(np.all(pooled_image == [[6.0, 8.0]])) | ||
|
|
There was a problem hiding this comment.
Here you can also add a check for the shape
Added docs, torch, tests for math.maxpooling.