Unified Scale projections and fixed black border #103

h-spiess · 2025-10-21T13:52:03Z

Scale projections had seperate implementations where the black border was only fixed for ScaleFixed. Unified implementations and fixed black borders for all projections.

PR Checklist

Tests are added
Documentation, if applicable

…g documentation

…r ScaleRatio

h-spiess · 2025-10-21T13:55:01Z

This fixes #102

h-spiess · 2025-10-21T13:56:34Z

Example from tests on master vs. this commit:

current master:

this commit:

paulnovo

Thank you, this is great. Consolidating the scale projections makes a lot of sense. I added just one comment/question.

paulnovo · 2025-10-23T12:58:23Z

test/projective/affine.jl

            tkeypoints = apply(tfm, keypoints)
-            @test getbounds(timage).rs == (0:25, 0:25)
+            @test !any(isnan.(timage |> itemdata))
+            @test getbounds(timage).rs == (2:26, 2:26)


Transforming a 50x50 image with ScaleRatio((1/2, 1/2)), I would expect this to be (1:25, 1:25). With your other fixes, and using a 0.5 offset instead of 1.0 in projectionbounds, this can be adjusted to (1:25, 1:25):

--- a/src/projective/affine.jl +++ b/src/projective/affine.jl @@ -88,7 +88,7 @@ end function projectionbounds(tfm::ScaleFixed{N}, P, bounds::Bounds{N}; randstate = nothing) where N bounds_ = transformbounds(bounds, P) - return offsetcropbounds(tfm.sizes, bounds_, ntuple(_ -> 1., N)) + return offsetcropbounds(tfm.sizes, bounds_, ntuple(_ -> 0.5, N)) end

Would this reintroduce the black border you are trying to fix? (it does look like all the other unit tests pass with this change).

Thanks for checking out the PR. I'm fine with this change. I've just sticked to an offset of 1.0 as it has been used before and is still used for the translation in getprojection. I guess they don't have to be consistent?

See:

DataAugmentation.jl/src/projective/affine.jl

Lines 52 to 65 in 3437276

upperleft = SVector{N, Float32}(minimum.(bounds.rs)) .- 0.5

P = scaleprojection(Tuple(ratio for _ in 1:N))

if any(upperleft .!= 0)

P = P ∘ Translation((Float32.(P(upperleft)) .+ 0.5f0))

end

return P

end

function projectionbounds(tfm::ScaleKeepAspect{N}, P, bounds::Bounds{N}; randstate = nothing) where N

origsz = length.(bounds.rs)

ratio = maximum((tfm.minlengths) ./ origsz)

sz = round.(Int, ratio .* origsz)

bounds_ = transformbounds(bounds, P)

bs_ = offsetcropbounds(sz, bounds_, ntuple(_ -> 0.5, N))

vs.

DataAugmentation.jl/src/projective/affine.jl

Lines 84 to 95 in 3437276

upperleft = SVector{N, Float32}(minimum.(bounds.rs)) .- 1

P = scaleprojection(ratios)

if any(upperleft .!= 0)

P = P ∘ Translation(-upperleft)

end

return P

end

function projectionbounds(tfm::ScaleFixed{N}, P, bounds::Bounds{N}; randstate = nothing) where N

bounds_ = transformbounds(bounds, P)

return offsetcropbounds(tfm.sizes, bounds_, ntuple(_ -> 1., N))

I've added it but it somehow screws up two consecutive scalings. I will try to resolve the issue.

…ling transformations

paulnovo

Thanks for digging into the bounds ranges, this is really great. I just left a couple of small comments and tweaks. But this is looking ready to go.

paulnovo · 2025-10-27T23:03:21Z

test/projective/affine.jl

+            timg = apply(tfm, Image(img)) |> itemdata
+            @test timg |> size == (32, 48)
+            @test !any(isnan.(timg))


Check bounds ranges here too:

Suggested change

timg = apply(tfm, Image(img)) |> itemdata

@test timg |> size == (32, 48)

@test !any(isnan.(timg))

timg = apply(tfm, Image(img))

@test getbounds(timg).rs == (1:32, 1:48)

@test !any(isnan.(timg |> itemdata))

paulnovo · 2025-10-27T23:04:55Z

test/projective/affine.jl

+            timg = apply(tfm, Image(img)) |> itemdata
+            @test timg |> size == (32, 32)
+            @test !any(isnan.(timg))


Same here

Suggested change

timg = apply(tfm, Image(img)) |> itemdata

@test timg |> size == (32, 32)

@test !any(isnan.(timg))

timg = apply(tfm, Image(img))

@test getbounds(timg).rs == (1:32, 1:32)

@test !any(isnan.(timg |> itemdata))

paulnovo · 2025-10-27T23:06:57Z

test/projective/affine.jl

+
+        @testset ExtendedTestSet "ScaleRatioTwice" begin
+            tfm = ScaleRatio((4/5, 4/5)) |> ScaleRatio((1/2, 1/2))
+            @show getbounds(image).rs


Please remove the @show

paulnovo · 2025-10-28T12:58:53Z

src/projective/compose.jl

+    @assert length(composed.tfms) == length(randstate)
+    P = CoordinateTransformations.IdentityTransformation()
+    for (tfm, r) in zip(composed.tfms, randstate)
+        P_tfm = getprojection(tfm, bounds; randstate = r)
+        bounds = projectionbounds(tfm, P_tfm, bounds; randstate = r)
+        P = P_tfm ∘ P
+    end
+    return bounds


Oh wow, ok. Nice catch here. We might eventually want to think about just writing an apply for ComposedProjectiveTransform so we don't calculate P twice (here and in getprojection), but your approach is probably the safest way forward.

paulnovo · 2025-10-28T13:03:39Z

test/projective/affine.jl

            timage = apply(tfm, image)
            tkeypoints = apply(tfm, keypoints)
+            @test !any(isnan.(timage |> itemdata))
            @test length.(getbounds(timage).rs) == (25, 25)


Would you mind adding checks to the full range range in these tests, not just the size. Down the road, I can image the ranges becoming incorrect again without a test

Suggested change

@test length.(getbounds(timage).rs) == (25, 25)

@test getbounds(timage).rs == (1:25, 1:25)

h-spiess added 2 commits October 21, 2025 14:56

unifying scaling projections, same black border offset for all, fixin…

959d763

…g documentation

Scale tests include test for black borders and updated bounds test fo…

3dff4a8

…r ScaleRatio

h-spiess changed the title ~~Unified Scale projections and fixed black bordern~~ Unified Scale projections and fixed black border Oct 21, 2025

paulnovo reviewed Oct 23, 2025

View reviewed changes

update projectionbounds to have nicer bounds and enable composing sca…

3f63255

…ling transformations

paulnovo reviewed Oct 28, 2025

View reviewed changes

	upperleft = SVector{N, Float32}(minimum.(bounds.rs)) .- 0.5
	P = scaleprojection(Tuple(ratio for _ in 1:N))
	if any(upperleft .!= 0)
	P = P ∘ Translation((Float32.(P(upperleft)) .+ 0.5f0))
	end
	return P
	end

	function projectionbounds(tfm::ScaleKeepAspect{N}, P, bounds::Bounds{N}; randstate = nothing) where N
	origsz = length.(bounds.rs)
	ratio = maximum((tfm.minlengths) ./ origsz)
	sz = round.(Int, ratio .* origsz)
	bounds_ = transformbounds(bounds, P)
	bs_ = offsetcropbounds(sz, bounds_, ntuple(_ -> 0.5, N))

	upperleft = SVector{N, Float32}(minimum.(bounds.rs)) .- 1
	P = scaleprojection(ratios)
	if any(upperleft .!= 0)
	P = P ∘ Translation(-upperleft)
	end
	return P
	end


	function projectionbounds(tfm::ScaleFixed{N}, P, bounds::Bounds{N}; randstate = nothing) where N
	bounds_ = transformbounds(bounds, P)
	return offsetcropbounds(tfm.sizes, bounds_, ntuple(_ -> 1., N))

	@test length.(getbounds(timage).rs) == (25, 25)
	@test getbounds(timage).rs == (1:25, 1:25)

Uh oh!

Uh oh!

Unified Scale projections and fixed black border #103

Are you sure you want to change the base?

Unified Scale projections and fixed black border #103

Uh oh!

Conversation

h-spiess commented Oct 21, 2025

PR Checklist

Uh oh!

h-spiess commented Oct 21, 2025

Uh oh!

h-spiess commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulnovo left a comment

Choose a reason for hiding this comment

Uh oh!

paulnovo Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-spiess Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-spiess Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulnovo left a comment

Choose a reason for hiding this comment

Uh oh!

paulnovo Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

paulnovo Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

paulnovo Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

paulnovo Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

paulnovo Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

h-spiess commented Oct 21, 2025 •

edited

Loading

paulnovo Oct 23, 2025 •

edited

Loading

h-spiess Oct 24, 2025 •

edited

Loading

h-spiess Oct 24, 2025 •

edited

Loading