-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support for quantitative evaluation of deconvolutions and addition of the inital
argument to supply start values.
#33
Conversation
…ve evaluation of deconvolutions. Also added an example.
Codecov Report
@@ Coverage Diff @@
## master #33 +/- ##
==========================================
- Coverage 92.71% 92.15% -0.56%
==========================================
Files 14 14
Lines 535 599 +64
==========================================
+ Hits 496 552 +56
- Misses 39 47 +8
Continue to review full report at Codecov.
|
Thanks for the PR. I was wondering, why do we need now Also I would feel more happy if code coverage increases instead of decreasing. Can you check with CodeCov which are the missing lines and add maybe simplistic tests? We should ensure that the code is at least running. Checking for meaningful values is hard, I admit. |
src/deconvolution.jl
Outdated
center_set!(rec0, one_arr) | ||
rec0 .*= mean(measured) | ||
fill!(one_arr, mean(measured)) | ||
center_set!(rec0, ones(csize) .* initial) |
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.
Why is ones(csize)
needed?
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.
Since I wanted the user to be able to call deconvolution
also with a single number (such as the mean or 0.0) for initialization.
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.
Did you try without ones(csize)
?
Maybe center_set!
handles that correctly.
src/analysis_tools.jl
Outdated
end | ||
end | ||
idx += 1 | ||
false |
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.
false? Why?
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.
This is specific to the callback. If it returns true
the optimizer is told to stop.
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.
maybe add that as a comment and with an explicit return false
Before it was a bit inconsistent with the iterations (default 20 for |
New commit with hopefully everything fixed. Also the code coverage should have been increased. |
…re sense for unnormalized data.
In the analysis_tools.jl support for quantitative evaluation of deconvolutions was added via returning an option structure to be provided to the deconvolution routine. The new function
options_trace_deconv
returns an opt_options structure to be supplied todeconvolution
which then does all the bookkeeping. It also returns a query function that then provides the results.In addition the "initial" argument was added to allow starting the iterations with a value or array of the user's choice.
The tests of the new functions makes use of this
initial
argument by supplying the ground truth and checking for the metrics to yield 1.0 and 0.0 respectively.