Test regularizationtools v0.6 #107
Conversation
|
@jjstickel can you help with this update? |
|
Sure. Some of the numerical minimization methods changed in RegularizationTools that are probably causing some slight differences in the results. I'll try to take a look sometime in the next few days. |
|
@ChrisRackauckas I've made changes to work with RegularizationTools-0.6 in a local branch pulled from your branch here. However, I do not have permission to commit back to this repository. Do I push the branch to my fork and make a new pull request? This is my first time trying to contribute to pull request initiated by someone else. |
|
@ChrisRackauckas, I created PR #108 to bring in my changes to your branch and this PR. Please review. |
revisions for RegularizationTools 0.6
| @@ -37,33 +37,33 @@ tolerance = 1e-6 | |||
| @test isapprox(A.û, ans, rtol=tolerance) | |||
| # L-curve to determine λ | |||
| A = RegularizationSmooth(uₒ,tₒ; alg=:L_curve) | |||
| @test isapprox(A.λ, 0.3339750913481082, rtol=tolerance) | |||
| ans = [0.3038273346447746 0.3695574797289306 0.7261740581164446 0.8945169297047363 0.9100773683859941 0.9048101771171335 0.8663722295718835 0.8188893880450858 0.7520030529566242 0.641893306140736 0.35912131844587486 0.12553463033078033 -0.45446129364457777 -0.8246966349034558 -1.118321479210823]' | |||
| @test isapprox(A.λ, 0.9536286111306728, rtol=tolerance) | |||
There was a problem hiding this comment.
@jjstickel Is it expected that λ changes so much?
There was a problem hiding this comment.
Thank you for the review. It turns out that the example I chose here is not the most robust, and neither is the L-curve method for finding the optimal lambda. So, switching to optimizing over log(lambda) (upstream change) resulted in a different optimal lambda.
test/regularization.jl
Outdated
| ans = [0.25185799867326253 0.32130333169345704 0.6679088195845279 0.927260505998722 0.9914594632495953 1.0028090695917116 0.9749355187531624 0.9254679051053563 0.8291064785367862 0.6493316082309926 0.2324329561930743 0.07635651700236243 -0.5564454824473795 -0.817911933888329 -1.1130193817730065]' | ||
| # arbitrary weights for wls (and fixed λ, GCV not working well for some of these) | ||
| A = RegularizationSmooth(uₒ,tₒ, nothing, collect(1:npts); λ=1e-1, alg=:fixed) | ||
| # @test isapprox(A.λ, 0.1447077651169621, rtol=tolerance) |
There was a problem hiding this comment.
Should the value be updated instead of the test commented out?
There was a problem hiding this comment.
Similarly, optimization via GCV(lambda) with these weight vectors is now finding a lower value for lambda that is not likely what most data scientists would want. Hence, I used fixed lambda now for these couple of tests. The next addition I plan to make (when I have time again) will be to allow the user to provide min and max values for lambda (rather than an initial guess). Then a minimum value can be specified to prevent GCV from settling on the impractically small value of lambda.
There was a problem hiding this comment.
Then I'd mark these as @test_broken for the time being. That makes it more visible that they should be revisited.
There was a problem hiding this comment.
I'm not yet familiar with @test_broken. Is the utility that it will still let you know if an exception occurs?
Even so, I'm not sure that's the right thing to do here. The previous test will never be correct now, and it is not necessarily wrong to test here with fixed lambda. Even with my planned updates, this test would still be valid as well, although I expect to change it when those updates occur. But if you still prefer @test_broken, I am happy to make that change -- let me know.
There was a problem hiding this comment.
Is the utility that it will still let you know if an exception occurs?
It's a way to track if a test returns false. It will show up in the summary and it will throw if the test suddenly passes. It's not for testing exceptions though. For that you'd use @test_throws
Either make it a passing test, a @test_broken, or delete the test if it's not relevant. My objection is to having the test commented out.
There was a problem hiding this comment.
Got it. I made another PR to this branch (#109).
test/regularization.jl
Outdated
| @test isapprox(A.λ, 0.01920769986569919, rtol=tolerance) | ||
| ans = [0.16161677980889166 0.34630920002600707 0.6469896059918103 0.9219255811754361 1.0001382485019457 1.0260666135098766 1.0278670301547874 0.9823839714037528 0.8512856706983483 0.6416080323126064 0.1892473613483678 0.10264529505906245 -0.5680977857976282 -0.8075007645180948 -1.1168524120952026]' | ||
| A = RegularizationSmooth(uₒ,tₒ, nothing, wls, wr; λ=1e-1, alg=:fixed) | ||
| # @test isapprox(A.λ, 0.01920769986569919, rtol=tolerance) |
No description provided.