JuliaImages / ImageTransformations.jl

Geometric transformations on images for Julia
Other
46 stars 27 forks source link

Restrict rounding to the computed corners #143

Closed timholy closed 3 years ago

timholy commented 3 years ago

Rounding the transformation makes assumptions about the nature of these transformations. However, in principle any kind of function mapping points to new points should be supported. It seems safer to perform rounding on the result of the transformation.

This will require https://github.com/JuliaMath/Interpolations.jl/pull/451 to pass the Lanczos tests.

The other test failure is because we're testing "fill" boundary conditions and without rounding the result is just barely beyond-the-edge. Any thoughts on how you want to approach that? One option would be to slightly round the coordinates before interpolation; a reasonably fast way to do that might be

__round(x::Float64) = Float64(Float32(x))
__round(x::Float32) = Float32(Float16(x))    # surprisingly, this is ~1ns also

but that will trigger a cascade of other small precision errors.

timholy commented 3 years ago

One option would be to slightly round the coordinates before interpolation

I'm supportive of this...That said, fixing the ImageTransformation tests seems sufficient for this PR IMO.

I like that idea---I might mark it as @test_broken or @test_skip for the meantime. It occurs to me that there's a simpler approach: define a FilledExtrapolation type that allows a small bit of "buffering" at the edges. The only problem basically gets caused when the evaluation point is 0.99999999... rather than 1.0 (where 1.0 is the edge of the image), and so a tiny bit of grace specifically at the edges seems like it would solve the problem while not reducing precision elsewhere. (The more I think about it, the more generic rounding would be a really bad idea for things like autodiff.)

`# surprisingly, this is ~1ns also

Is this because Float16 in Julia is implemented in software?

That's why I'm surprised, but it turns out this specific transformation comes down to CPU instructions:

julia> @code_typed __round(0.8f0)
CodeInfo(
1 ─ %1 = Base.fptrunc(Base.Float16, x)::Float16
│   %2 = Base.fpext(Base.Float32, %1)::Float32
└──      return %2
) => Float32

and these boil down to CPU instructions:

codecov[bot] commented 3 years ago

Codecov Report

Merging #143 (e424b3c) into master (226d504) will increase coverage by 0.06%. The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   92.09%   92.15%   +0.06%     
==========================================
  Files           8        8              
  Lines         215      204      -11     
==========================================
- Hits          198      188      -10     
+ Misses         17       16       -1     
Impacted Files Coverage Δ
src/compat.jl 80.00% <ø> (-3.34%) :arrow_down:
src/warp.jl 100.00% <ø> (ø)
src/warpedview.jl 85.00% <ø> (-0.72%) :arrow_down:
src/autorange.jl 92.68% <80.00%> (+0.84%) :arrow_up:
src/invwarpedview.jl 97.14% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 226d504...e424b3c. Read the comment docs.

johnnychen94 commented 3 years ago

I'll finish my other two PRs in the next few days so we don't block the v0.9 release permanently.