unwrap: Initialize image reliability in deterministic order #617
Conversation
RomanHargrave
changed the title
RomanHargrave
force-pushed
the
roman/deterministic-unwrap
branch
2 times, most recently
from
508a6aa to
23f4da1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #617 +/- ##
=======================================
Coverage 97.96% 97.96%
=======================================
Files 19 19
Lines 3250 3251 +1
=======================================
+ Hits 3184 3185 +1
Misses 66 66 ☔ View full report in Codecov by Sentry. |
wheeheee
force-pushed
the
roman/deterministic-unwrap
branch
from
d40855b to
2e77879
Compare
|
|
||
| f_uw = f_wraparound2.(grid, grid', reshape(grid, 1, 1, :)) | ||
| f_wr = f_uw .% (2convert(T, π)) | ||
| uw_test = unwrap(f_wr, dims=1:3) |
There was a problem hiding this comment.
while doing a bit of cleanup i stumbled upon this. i think it should have been called with circular_dims? so I amended it.
There was a problem hiding this comment.
Other than that test I'm not completely sure about, I think this is ok and I'll merge this in a while.
@martinholters any comments?
There was a problem hiding this comment.
Unrelated, but the use of
@threadshere seems like it is of limited benefit - there does not seem to be any computationally intense work inside the loop.
Agree. Now it's just the allocation of Pixels that happens threaded, I believe? Is that even beneficial, or does the allocation then serialize things again by using locks anyway, and it's just the trivial initialization that runs in parallel?
|
|
||
| # Initialize reliability values before going parallel. This ensures that | ||
| # reliability values are generated in a deterministic order. | ||
| rels = rand(rng, Float64, size(wrapped_image)) | ||
|
|
||
| Threads.@threads for i in eachindex(wrapped_image, pixel_image, rels) | ||
| pixel_image[i] = Pixel(wrapped_image[i], rels[i]) |
There was a problem hiding this comment.
Is this @threads even worth it or could we just keep the old code but with Threads.@threads removed?
There was a problem hiding this comment.
Oops, missed that. Ran some quick benchmarks, and not sure why, but on average it seems to be slightly faster with the @threads, but it's within the error.
julia> @benchmark unwrap(A; dims=1:2) setup=A=rand(500,500)
BenchmarkTools.Trial: 47 samples with 1 evaluation. # PR
Range (min … max): 90.717 ms … 143.875 ms ┊ GC (min … max): 0.00% … 32.85%
Time (median): 103.035 ms ┊ GC (median): 8.80%
Time (mean ± σ): 105.516 ms ± 12.979 ms ┊ GC (mean ± σ): 11.29% ± 9.20%
█ ▅ ▂ ▂
██▁██▁█▁▅█▁█▅████▁▅█▁▁▅█▁▅▅▅▁▁▁▅▁▁▁▁▅▁▁▁▁▁█▁▁▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▅ ▁
90.7 ms Histogram: frequency by time 144 ms <
Memory estimate: 60.97 MiB, allocs estimate: 250108.
BenchmarkTools.Trial: 44 samples with 1 evaluation. # PR without @threads init
Range (min … max): 92.638 ms … 151.973 ms ┊ GC (min … max): 0.00% … 31.74%
Time (median): 109.116 ms ┊ GC (median): 13.32%
Time (mean ± σ): 113.244 ms ± 17.772 ms ┊ GC (mean ± σ): 14.86% ± 11.17%
▂ █ ▂ ▂
█▅█▅▅▅▁▁█▅▁▁▁▁█████▁█▁▁▅▁▁▁▁▅▅▁▅▅▁▁▅▅▅▁▁▁▁▁▁▁▁▅▁▁▁▁▅▁▁▅▁█▁▁▅▅ ▁
92.6 ms Histogram: frequency by time 152 ms <
Memory estimate: 60.96 MiB, allocs estimate: 250066.
julia> @benchmark unwrap(A; dims=1:3) setup=A=rand(50,50,50)
BenchmarkTools.Trial: 77 samples with 1 evaluation. # PR
Range (min … max): 56.739 ms … 95.696 ms ┊ GC (min … max): 0.00% … 39.25%
Time (median): 65.154 ms ┊ GC (median): 10.08%
Time (mean ± σ): 64.903 ms ± 7.279 ms ┊ GC (mean ± σ): 9.90% ± 8.89%
█▃▃
▇███▅▄▁▅▄▁▄▄▁▁▁▅▄▁▄▁▁▄▅▄▅▅▇▄▄▇▁▅▅▁▇▄▅▄▄▄▅▇▁▁▁▄▁▄▁▁▁▁▁▁▁▁▄▄▅ ▁
56.7 ms Histogram: frequency by time 78.8 ms <
Memory estimate: 39.94 MiB, allocs estimate: 125108.
BenchmarkTools.Trial: 72 samples with 1 evaluation. # PR without @threads init
Range (min … max): 58.111 ms … 106.682 ms ┊ GC (min … max): 0.00% … 42.53%
Time (median): 64.032 ms ┊ GC (median): 0.00%
Time (mean ± σ): 68.962 ms ± 11.368 ms ┊ GC (mean ± σ): 12.67% ± 12.33%
█▅▅
███▅▆▅▃▃▅▃▁▁▁▁▁▁▁▅▅▆▃▅▅▆▆▃▆▃▁▁▅▃▁▁▁▃▁▁▁▁▁▁▅▃▁▁▁▃▁▁▁▁▁▁▁▁▁▁▃▃ ▁
58.1 ms Histogram: frequency by time 100 ms <There was a problem hiding this comment.
Then I'd tend to the simpler implementation, but no strong opinion here.
There was a problem hiding this comment.
I would just keep it. In fact I think we could also add Threads.@threads to for i in fi:li in the first loop of calculate_reliability...
for invalid input; valid input should be resistant to random noise
wheeheee
force-pushed
the
roman/deterministic-unwrap
branch
from
2086623 to
3de3387
Compare
init_image()presently callsrand()once per pixel to initialize reliability values. As this is done inside an@threadsloop, this does not happen in a deterministic order, in turn causingunwrapto produce measurably different outputs when executed for the same input and with a prepared PRNG. To remedy this, I moved reliability value generation outside of the loop, which stabilized the output - now there is zero variation between applications (with a fresh PRNG).Unrelated, but the use of
@threadshere seems like it is of limited benefit - there does not seem to be any computationally intense work inside the loop.Fixes #616