-
Notifications
You must be signed in to change notification settings - Fork 7
Add Denoising Problem #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Denoising Problem #43
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
- Coverage 99.16% 92.75% -6.42%
==========================================
Files 10 12 +2
Lines 360 469 +109
==========================================
+ Hits 357 435 +78
- Misses 3 34 +31 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all this. Do obj
and grad!
allocate? could you show some benchmarks?
Sure there it is : julia> model, sol = denoising_model((n, m), (n_p, m_p), kz, kt);
julia> x,y = ones(n * m), rand(n * m);
julia> @benchmark obj($model, $x)
BenchmarkTools.Trial: 2017 samples with 1 evaluation.
Range (min … max): 1.893 ms … 23.679 ms ┊ GC (min … max): 0.00% … 65.07%
Time (median): 2.307 ms ┊ GC (median): 0.00%
Time (mean ± σ): 2.479 ms ± 1.012 ms ┊ GC (mean ± σ): 6.94% ± 12.51%
▂ ▃█▂
▆█▇███▅▆▇▄▄▁▃▄▄▁▃▃▃▁▁▁▁▁▃▁▁▁▁▃▁▁▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▅█▇ █
1.89 ms Histogram: log(frequency) by time 7.45 ms <
Memory estimate: 8.21 MiB, allocs estimate: 585.
julia> @benchmark grad!($model, $y, $x)
BenchmarkTools.Trial: 1061 samples with 1 evaluation.
Range (min … max): 3.418 ms … 19.016 ms ┊ GC (min … max): 0.00% … 78.02%
Time (median): 4.221 ms ┊ GC (median): 0.00%
Time (mean ± σ): 4.713 ms ± 1.945 ms ┊ GC (mean ± σ): 10.12% ± 15.49%
▁▅██▅▃▁
████████▇▄▄▁▁▁▁▁▄▁▁▁▁▁▄▄▄▅▆▇█▇▇▄▄▁▄▁▁▅▁▄▁▁▁▁▄▁▁▁▁▄▄▄▁▄▄▁▆▆ █
3.42 ms Histogram: log(frequency) by time 14.9 ms <
Memory estimate: 17.42 MiB, allocs estimate: 1165. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! I also added a few comments to try to reduce some allocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be possible to reduce the allocations even more the allocation by preallocating and using in-place fft etc..., but I'm not sure that they exist. Maybe we can merge and open an issue saying that we can improve the allocations if needed.
a168a30
to
abe7265
Compare
03b7c5b
to
bb4ea62
Compare
@dpo normalement j'ai tenu compte de tous tes commentaires dans cette PR |
Add missing packages
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
7fe4418
to
9588103
Compare
Co-authored-by: Dominique <[email protected]>
9588103
to
72874a3
Compare
…pendencies have been imported
include("denoising_data.jl") | ||
|
||
function denoising_model(shape, shape_p, KERNEL_SIZE, KERNEL_SIGMA = 1.5) | ||
sigma = 10^-3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this model only work in Float64? Could sigma
be made generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the literature, no one seemed to be interested by this problem at lower precision.
I'm also afraid, that the wavelet or Fourier transform might not be generic. I should check.
|
||
function obj(x) | ||
y .= W_T(x) | ||
y .= H(y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there in-place versions of H
and W_T
? Currently, there are lots of allocations here.
@MohamedLaghdafHABIBOULLAH Could you please rebase this PR? |
No description provided.