Open ingenieroariel opened 10 years ago
Applying the following patch (to undo the optional change) fixes the ERROR, but I do not know what the right generic solution with keywords arguments is:
diff --git a/src/regression.jl b/src/regression.jl
index c9e3e49..170385d 100644
--- a/src/regression.jl
+++ b/src/regression.jl
@@ -2,18 +2,18 @@ export nnlsq, lasso, ridge, elastic_net
# admm_consensus accepts z0 (initial guess of solution)
# and params as optional keyword arguments
-function nnlsq(A,b; kwargs...)
- return admm_consensus([prox_pos,make_prox_lsq(A, b, params.rho)],size(A,2); kwargs...)
+function nnlsq(A,b; params=Params(), kwargs...)
+ return admm_consensus([prox_pos,make_prox_lsq(A, b, params.rho)],size(A,2);params=params, kwargs...)
end
-function lasso(A,b,lambda; kwargs...)
- return admm_consensus([make_prox_l1(lambda, params.rho),make_prox_lsq(A, b, params.rho)],size(A,2); kwargs...)
+function lasso(A,b,lambda; params=Params(), kwargs...)
+ return admm_consensus([make_prox_l1(lambda, params.rho),make_prox_lsq(A, b, params.rho)],size(A,2);params=params, kwargs...)
end
-function ridge(A,b,lambda; kwargs...)
- return admm_consensus([make_prox_l2(lambda, params.rho),make_prox_lsq(A, b, params.rho)],size(A,2); kwargs...)
+function ridge(A,b,lambda; params=Params(), kwargs...)
+ return admm_consensus([make_prox_l2(lambda, params.rho),make_prox_lsq(A, b, params.rho)],size(A,2); params=params, kwargs...)
end
-function elastic_net(A,b,lambda1,lambda2; kwargs...)
- return admm_consensus([make_prox_l1(lambda1, params.rho),make_prox_l2(lambda2, params.rho),make_prox_lsq(A, b, params.rho)],size(A,2); kwargs...)
-end
\ No newline at end of file
+function elastic_net(A,b,lambda1,lambda2; params=Params(), kwargs...)
+ return admm_consensus([make_prox_l1(lambda1, params.rho),make_prox_l2(lambda2, params.rho),make_prox_lsq(A, b, params.rho)],size(A,2); params=params, kwargs...)
+end
The regression functions now accepts two keyword arguments: params and z. The optional argument z is an initial guess of the solution. Giving a good guess tends to improve convergence speed.
My apologies, I should have explained better. When I run the unit tests I get:
➜ ParallelSparseRegression git:(master) julia test/test.jl
ERROR: params not defined
while loading /Users/x/.julia/ParallelSparseRegression/test/test.jl, in expression starting on line 15
Which means the current use of kwargs is broken after removing the params keyword from the method signature. I will find the culprit and submit a pull request.
The problem is accessing params as a variable without getting it out from kwargs:
function example(x;kwargs...)
print(kwargs); print(params)
end
julia> example(1;params=43, oye=43)
(params,43)
(oye,43)
ERROR: params not defined
The main issue in our case, is that params.rho needs to be accessed to create the proxs but params is now optional.
I have a crazy idea, what if we get rid of the params dict and just let keywords propagate? That will improve the syntax of the functions in regressions:
function lasso(A,b,lambda; rho=1, kwargs...)
return admm_consensus([make_prox_l1(lambda, rho),make_prox_lsq(A, b, rho)],size(A,2); rho=rho, kwargs...)
end
without a need for a hack like the one below:
function ridge(A,b,lambda; kwargs...)
kwargs_dict = {k=>v for (k,v) in kwargs}
rho = 1
if :params in kwargs_dict
rho = kwargs_dict[:params].rho
end
return admm_consensus([make_prox_l2(lambda, rho),make_prox_lsq(A, b, rho)],size(A,2); kwargs...)
end
Running the above file returns now: