Features: Implemented ChannelFirst2d#354
Conversation
Pwhsky
changed the title
| def __init__( | ||
| self: Feature, | ||
| axis: int = -1, | ||
| axis: PropertyLike[int] = -1, |
There was a problem hiding this comment.
Should axis be PropertyLike[int]? I don't really see why it would ever be something other than a fixed integer.
deeptrack/features.py
Outdated
| warnings.warn( | ||
| "ChannelFirst2d is deprecated and may be removed in a future release. " | ||
| "The current implementation is not guaranteed to be exactly " | ||
| "equivalent to prior implementations. " |
There was a problem hiding this comment.
I think that a comma is missing after the string. Without it I get an error
There was a problem hiding this comment.
Also, a line goes beyond 79 characters
deeptrack/features.py
Outdated
| @@ -6147,19 +6147,21 @@ class ChannelFirst2d(Feature): | |||
|
|
|||
| This feature rearranges the axes of a 3D image so that the specified axis | |||
There was a problem hiding this comment.
Some lines have blank spaces at the end. Check it and remove the spaces.
deeptrack/features.py
Outdated
| self: Feature, | ||
| image: np.ndarray, | ||
| image: NDArray | torch.Tensor, | ||
| axis: int, |
There was a problem hiding this comment.
If "axis = -1" in the init, shouldn't it also be so here in the "get"? Or remove it in both cases.
deeptrack/tests/test_features.py
Outdated
|
|
||
| input_image_3d = torch.rand(10, 20, 3) | ||
| output_image_3d = channel_first_feature.get(input_image_3d, | ||
| axis=-1) |
There was a problem hiding this comment.
Does this work for you? When I try to run this in my script I get an error when using a 3d torch tensor and axis=-1
There was a problem hiding this comment.
Thank you for pointing this out, I have added a fix for this that passes the tests properly.
| output_image_3d = channel_first_feature.get(input_image_3d, | ||
| axis=-1) | ||
| self.assertEqual(tuple(output_image_3d.shape), (3, 10, 20)) | ||
|
|
There was a problem hiding this comment.
I think that it would be nice to add some test with the actual values of the arrays and tensors. They can of course be small (e.g. [[1, 2, 3], [4, 5, 6]])
| @@ -6287,7 +6305,7 @@ class Upscale(Feature): | |||
|
|
|||
| Methods | |||
There was a problem hiding this comment.
I think that it becomes easier to review if you only put one class in each PR, so a separate one for the "Upscale" would have been nicer
There was a problem hiding this comment.
To me it doesn't seem like "Upscale" is ready yet. Let me know when you want a review on it
| Methods | ||
| ------- | ||
| `get(image: np.ndarray | Image, factor: int | tuple[int, int, int], **kwargs) -> np.ndarray` | ||
| `get(image: np.ndarray | Image | torch.tensor, factor: int | tuple[int, int, int], **kwargs) -> np.ndarray | torch.tensor` |
There was a problem hiding this comment.
This line, and also some other line, is longer than 79 characters
| image: np.ndarray | ||
| image: NDArray | torch.Tensor | ||
| The input image to process. Can be 2D or 3D. | ||
| axis: int |
There was a problem hiding this comment.
Maybe add that "axis" doesn't affect anything for 2D images
| self: Feature, | ||
| image: np.ndarray, | ||
| image: np.ndarray | torch.tensor, | ||
| factor: int | tuple[int, int, int], |
There was a problem hiding this comment.
If there is the default in init, schouldn't we have the same default value here?
3 participants
This PR implements ChannelFirst2d extended to handle torch tensors, as well as adding unit tests for torch tensors.