Add 1D-Total variation prox #128
Conversation
There was a problem hiding this comment.
Nice! Thanks for this addition! I left some comments inline.
I think two things are missing:
- An entry for this feature in the documentation (just add the type name eg below this line https://github.com/JuliaFirstOrder/ProximalOperators.jl/master/docs/src/functions.md#L67)
- A test, for which you could eg add one or more test cases to this test file https://github.com/JuliaFirstOrder/ProximalOperators.jl/blob/master/test/test_optimality_conditions.jl This requires implementing a check for the optimality conditions of the prox problem: these are in eqn (7) in the paper, I think they can be evaluated starting from x* (the output point in the code) and using eqns (5) and (6) too. Unless I’m missing something?
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
==========================================
- Coverage 87.63% 87.33% -0.30%
==========================================
Files 77 78 +1
Lines 2418 2472 +54
==========================================
+ Hits 2119 2159 +40
- Misses 299 313 +14
Continue to review full report at Codecov.
|
|
I added now a test for the optimality conditions. Worked for me for a couple of test cases, though the approximately equal to 0 assertion is a bit tricky. If you have any suggestions let me know. |
There was a problem hiding this comment.
I almost forgot: to be able to use this, you need to include the source file from the main package script: https://github.com/JuliaFirstOrder/ProximalOperators.jl/blob/master/src/ProximalOperators.jl this is why tests are currently failing (the new type cannot be used)
How big of an issue is the assertion on u[end]? Does it fall significantly away from zero? Maybe you could add some (non random) test cases where the problem occurs, to better look into it
There was a problem hiding this comment.
First of all, good job and thanks for translating the algorithm from the paper into a GOTO-free version, that must have been a bit of a puzzle!
It looks like the test cases are not covering many of the algorithm branches (at least according to codecov’s comments). This may be due to the fact that the test cases are very small or not particularly relevant for this algorithm (like in case y ends up being equal to x, or a constant vector). One way to come up with better ones might be to:
- design a piecewise-constant signal, with a handful of change points, of large enough magnitude so that denoising doesn’t get rid of them
- add small magnitude noise, that would be killed by denoising
This way one should be able to have fixed (non-random) test instances of small size.
To nail what “small vs large” magnitudes are, one can do trial and error or try to understand this from the optimality conditions or algorithm logic, whatever is easier.
Sorry for this further ask :-) but I think it’s worth having test cases that cover the algorithm properly. If I have time I’ll try to come up with some test cases too!
|
So I played around a bit and it seems that some of the |
|
@lostella I added the "pathological case" from section 2A in the paper. Together with the other test cases, I think almost every clause but the first is now covered. But I don't really know how to get the last one. Did you have any success? |
Thanks! Didn’t get to it yet, but maybe it’s enough test cases already. I’ll let the tests run and if everything is green approve & merge |
I created the main file. Can you help me with what else I need to add? Also I am not familiar to programming in Julia, so if there are any best practices I am violating, let me know.