Make Flips ND compatible and fix ND ScaleKeepAspect, ScaleFixed and PinOrigin #75
Conversation
…ed and PinOrigin. Also add ScaleFixed and Zoom to the 3D test to catch the bug.
There was a problem hiding this comment.
Thanks for the contribution!
This looks good generally, I've left some comments. It seems that this only touches Flip, not Zoom like the title implies, right?
Other than the minor suggested changes, could you add some tests that test that these transformations behave correctly in 3D? Some sanity checks, i.e.
FlipX{3}()applied to a(10, 10, 1)image should be the same asFlipX{2}()to a(10, 10)image (dropping the last dimension). Same forFlipYFlipZ{3}()applied to a(10, 10, 1)image should be an identity operation
If you're testing this on 3D images, I would also be interested in some visual examples as an additional check. Let me know if you something is unclear!
src/projective/affine.jl
Outdated
| function reflectionmatrix(r) | ||
| A = SMatrix{2, 2, Float32}(cos(2r), sin(2r), sin(2r), -cos(2r)) | ||
| return round.(A; digits = 12) | ||
| end | ||
|
|
||
|
|
||
| """ | ||
| FlipDim{N}() |
There was a problem hiding this comment.
Think this should be
| FlipDim{N}() | |
| FlipDim{N}(dim) |
src/projective/affine.jl
Outdated
| FlipX(N) = FlipDim{N}(N==2 ? 2 : 1) | ||
| FlipY(N) = FlipDim{N}(N==2 ? 1 : 2) | ||
| FlipZ(N) = FlipDim{N}(3) |
There was a problem hiding this comment.
Can we make this so you call FlipX{N}()? The way it is I find it somewhat confusing that for FlipDim the type variable is the dimension of the data, where as for FlipXYZ the dimension of the data is the argument.
There was a problem hiding this comment.
yes makes sense, I changed it accordingly
…constructors, to match the FlipDim definition
|
Thank you for the feedback :) Yes you're right, I just added 2 tests for images where the flip dimension has size 1, but I ran into a |
itan1
changed the title
|
The arrays with singleton dimensions did not work well with Interpolations.jl, they give bounds errors during testing. Instead of the singleton tests I checked the flipped image pixel values by reverse indexing and added a test to check that double flipping results in the original image. |
|
@lorenzoh can this be merged? |
This adds missing documentation for the Flip transformations, and adds FlipDim to the reference page. Also, fixes the "Big Pipeline/3D" unit test that was failing due to not being able to compose two ComposedProjectiveTransforms.
|
I merged master into this branch and updated a few things in this branch. I can help with any additional needed changes to get this merged. If everything looks good, should I create a PR into this |
paulnovo
dismissed
lorenzoh’s stale review
Requested changes appear to be addressed, I would like to unblock this PR.
The transforms
RotateandReflectare still only 2D compatible.closes #74