imresize/imrotate fixed point
#129
Conversation
|
Maybe we can provide a struct FixedPoint{N}
p::CartesianIndex{N}
end
imresize(img, (512, 512), FixedPoint(1, 1))Another choice is to introduce a new name for every function, i.e., |
|
I think your solution nicely and obviously separates functions with a fixed point and without, so I'd proceed with it. I guess, it's going to be in a new file for further using in |
|
Always forget about tests until they fail... |
|
Another possible name can be |
src/resizing.jl
Outdated
| offset = cp.I .- new_size .* (cp.I .- topleft) .÷ size(original) .- 1 | ||
| resized = OffsetArray(similar(original, Tnew, new_size), offset) | ||
| imresize!(resized, original; kwargs...) | ||
| resized[cp] = original[cp] |
There was a problem hiding this comment.
I see that resized[cp] can be different from original[cp] because of the warp method we're using. But I don't think we need to overwrite resized[cp] here.
Looks like there's no fixed point at all 😢
src/centerpoint.jl
Outdated
| ```jldoctest | ||
| img[cp] == imgr[cp] | ||
| ``` |
There was a problem hiding this comment.
this jldoctest is definitely broken. Using
| ```jldoctest | |
| img[cp] == imgr[cp] | |
| ``` | |
| ```julia | |
| img[cp] == imgr[cp] | |
| ``` |
would be fine.
The more concerning issue here is that maybe there isn't such a fixed point concept at all. All we're introducing is a warp center point. Looks like it is this property that holds true:
CartesianIndices(img)[cp] == CartesianIndices(imgr)[cp]I'm in a bad state here and don't have enough bandwidth to think this carefully. Can you double-check it?
There was a problem hiding this comment.
Oh, I could make such a bad mistake, sorry. I will test it carefully.
There was a problem hiding this comment.
Didn't notice 'at all' and got the meaning of your comment wrong.
All we're introducing is a warp center point.
Honestly, that's what I was thinking about. Not to disturb your state, but can you give any tips of how this idea looks in your head? Othewise I'm afraid I don't understand.
There was a problem hiding this comment.
Sorry for the back-and-forth and the inaccuracy.
In https://evizero.github.io/Augmentor.jl/dev/operations/#Affine-Transformations, the scale/resize is no different from imresize. I was thinking of a generic solution to the Zoom operator there where the center point is invariant. The term "fixed point" showed itself in my head when I wrote down #121.
There was a problem hiding this comment.
Sorry for the delayed response, got stuck with exams. Unfortunately, I still can't fully understand your concerns. I made a poor visualization of what it does right now when I create a new image with imresize(img, FixedPoint(100, 100); ratio=0.5).
To me it looks like it works as expected and implements the Zoom operator, except it doesn't crop the image. Hopefully, my point is clear and makes sense.
There was a problem hiding this comment.
Looks good to me! I'll recheck next weekend (also busy with school stuff). In the meantime, if you can add up some tests when you're available it would be great.
| test_imresize_interface(img, (5,5), (5,5), CenterPoint(1, 1)) | ||
| test_imresize_interface(img, (20,20), CenterPoint(1, 1), ratio = 2) | ||
| test_imresize_interface(img, (20,10), CenterPoint(1, 1), ratio = (2, 1)) |
There was a problem hiding this comment.
This only makes sure the function gets called and outputs something. We also need to test if the output is correct, for example:
- test if
imresize(img, (20,20), CenterPoint(1, 1))works the same asimresize(img, (20,20)) - test if
imresize(img, (20, 20), CenterPoint(5, 5); method=Constant())[5, 5] == img[5, 5](If I'm right about the implementation, I'm not sure if it's[5, 5]here). - If it's not supported, test if
imresize(imag, (20, 20), CenterPoint(-10, -10))errors.
Also, OffsetArray is used widely in the ecosystem, so it would be great to also test OffsetArray(img, -1, -1) works normally for the above test cases.
There was a problem hiding this comment.
About testing if the CenterPoint doesn't change, without overriding the value it can be actully quite different, so either we should override it or it won't be ensured. Except this point I'm working on it.
There was a problem hiding this comment.
Yep, I also noticed this. BSpline(Constant()) is the nearest interpolation, I believe by using this method, it won't doesn't change the value?
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
==========================================
+ Coverage 91.74% 92.64% +0.89%
==========================================
Files 8 9 +1
Lines 218 231 +13
==========================================
+ Hits 200 214 +14
+ Misses 18 17 -1
Continue to review full report at Codecov.
|
|
@ginkulv Sorry for the delay! This fixed point thing is more complicated than I thought it could be, so instead of commenting here, I'll open a PR for it for discussion. |
Resolves #121.
First of all, I haven't implemented
imresize(img, inds, p::FixedPointType). It didn't make sense to me when I thought about it. As I understand it, the point of this transformation is adjusting the axes in the way that thefixed_pointis, well, fixed. And here we explicitly define the axes beforehand.The second thing is about
fixed_pointitself. When it's defined asUnion{Dims{N}, CartesianIndex}the following functions have the same set of arguments:when a
Tupleis passed. By this reason right now it only takesCartesianIndex.Please, correct me, if I'm wrong. If it looks fine to you, I'll move on writing tests and implementing it for
imrotate.