-
Notifications
You must be signed in to change notification settings - Fork 2
QR refactoring #27
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
base: main
Are you sure you want to change the base?
QR refactoring #27
Conversation
cortner
left a comment
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.
just some questions, nothing blocking I think...
src/BPDual.jl
Outdated
| function Base.show(io::IO, t::ASPTracer) | ||
| nsteps = length(t.iteration) | ||
| nvars = isempty(t.solution) ? 0 : length(t.solution[end].nzind) | ||
| println(io, "ASPTracer with $nvars active variables at final step.") |
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 would output it a bit differently, something like
ASPTracer(maxactive = ...)
or something like that.
src/BPDual.jl
Outdated
| cur_r_size +=1 | ||
| cur_r_size +=1 | ||
| if itn % refactor_freq == 0 && cur_r_size > 0 | ||
| F = qr!(S[:, 1:cur_r_size]) |
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.
is it really safe to do this in-place?
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.
guess it would be better to copy?
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.
if in-place is safe then in-place is better. I just wanted to be certain that you are certain about it. But it overwrite S; so if S is just a buffer that gets overwritten again before the next qr! then it's fine.
src/BPDual.jl
Outdated
| cur_r_size -=1 | ||
| cur_r_size -=1 | ||
| if itn % refactor_freq == 0 && cur_r_size > 0 | ||
| F = qr!(S[:, 1:cur_r_size]) |
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.
(same as above)
src/omp.jl
Outdated
| cur_r_size +=1 | ||
|
|
||
| if itn % refactor_freq == 0 && cur_r_size > 0 | ||
| F = qr!(S[:, 1:cur_r_size]) |
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.
(same)
|
|
||
| # Solve the basis pursuit problem | ||
| tracer = asp_bpdn(A, b, 0.0, loglevel =0); | ||
| tracer = asp_bpdn(A, b, 0.0, loglevel =0, refactor_freq =20); |
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.
should there be tests with and without refactor? Could one design a test that requires a refactor?
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 we could add a test where the columns of A are nearly linearly dependent..
Added a refactor frequency parameter:
refactor_freq.TO DO: add the automatic triggering condition based on max(R_ii)/min(R_ii).