-
Notifications
You must be signed in to change notification settings - Fork 7
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
ASP #78
ASP #78
Conversation
Thanks for the PR. This gives a minimal version that will make this ASP solver accessible. I think adding the model path to the result dict would be great. |
The other thing I would love to have is the following, maybe that can be done with a separate solver type
Maybe we can make this a separate PR - and I'd be happy to collaborate on that. I could see this becoming the main default, next to BLR. |
Yes, this sounds really nice. I will make another PR with this soon! 😄 |
src/solvers.jl
Outdated
function solve(solver::ASP, A, y; kwargs...) | ||
AP = A / solver.P | ||
tracer = asp_homotopy(AP, y; loglevel=0, traceFlag=true, kwargs...) | ||
xs = tracer[end][1] |
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.
Let's also revisit this: I don't think that selecting the final iterate is normally desired. Maybe there could be a few options given by an additional parameters
ASP(; P = I, select = ( :byerror, 1.2 ), params... )
ASP(; P = I, select = ( :bysize, 1000 ), params... )
ASP(; P = I, select = ( :final, ), params... )
(:byerror, 1.2)
would select the smallest active set fit that is within a factor 1.2 of the smallest fit error (not test error) achieved along the path. (:bysize, 1000)
would select the smallest error fit that that less than 1000 active basis functions; while (:final, )
would do exactly what you did.
The only question is what the default should be. Maybe there should be no default and the user has to specify the mode?
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.
I guess :bysize here doesn't really make sense unless we just integrate it into SmartASP and bring in test/validation sets, because using homotopy, the residual on the training set monotonically decreases so the smallest error fit that has <= 1000 active basis functions happens at actMax=1000.
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.
other than this, I think I've made all of the changes requested! Please let me know if I should change anything else :)
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.
monotonically decreases so the smallest error fit
ok, fair point. I need to think about it for a bit ...
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.
I think we should still add this option. I can envision that I want the solution with N parameters but have access to the model path up to 3*N parameters (or similar) for analysis ...
src/solvers.jl
Outdated
|
||
new_tracer = Vector{typeof(tracer[1])}(undef, length(tracer)) | ||
for i in 1:length(tracer) | ||
new_tracer[i] = (solver.P \ Array(tracer[i][1]), tracer[i][2]) |
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.
I'm confused here. I thought the solution path is stored as an array of sparse vectors?
this is now registered as 0.2.2 |
This PR integrates the sparse ASP solver into ACEfit.