Skip to content
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

Don't call factorize_linear_constraint every time in _update_equilibrium of ProximalProjection? #1404

Open
1 task
dpanici opened this issue Nov 20, 2024 · 8 comments · May be fixed by #1409
Open
1 task
Assignees
Labels
P3 Highest Priority, someone is/should be actively working on this performance New feature or request to make the code faster

Comments

@dpanici
Copy link
Collaborator

dpanici commented Nov 20, 2024

def _update_equilibrium(self, x, store=False):

  • profile an optimization and see if this is actually a bottleneck
@dpanici dpanici added the performance New feature or request to make the code faster label Nov 20, 2024
@dpanici
Copy link
Collaborator Author

dpanici commented Nov 20, 2024

This is not a true profile, but I threw "verbose=3" in the solve_options for an L=M=14 N=12 free bdry solve I am doing on della (with 8 threads requested so it should be multi-threaded), and the linear constraint projection of each equilibrum subproblem takes like 12.6 seconds (and seems like each time it takes about that long). Each subproblem solution time itself is on the same order of magnitude (about 12-14 seconds)

So I think this could be a significant timesaver, if we can just pass in the A matrix of the factorization to the _update_equilibrium steps and have it internally, if the A was passed in, just do the particular solution and not the factorization (will have to account for the different b but this should be doable)

@YigitElma
Copy link
Collaborator

Can you share your script so I can run it on nsys?

@f0uriest
Copy link
Member

I think we should add an update_b method to LinearConstraintProjection, similar to how we have an update_target method for most of the linear objectives.

Then, in proximal, we would call eq.solve with the already wrapped and updated linear constraint projection rather than passing in the constraints each time

@YigitElma
Copy link
Collaborator

YigitElma commented Nov 21, 2024

I will send some profiles but it is hard to see them probably. So, maybe I can show you on Zoom if you want.
Each script is here and I ran them on Della A100 with 32 core cpu.
proximal-lsq-exact-script.txt
lsq-auglag-script.txt

Optimization with lsq-auglag

image
It builds LinearConstraintProjection.build twice at the beginning, and there is no more factorize_linear_constraints operation. Taking jacobian and evaluating the function are very small portions of each iteration. Yes, there are multiple function evaluations inside "tr_subproblem" but it is on the order of couple hundred microseconds.
image

solve continuation_automatic

This part is even more complicated to see.
image
Basically, it goes like perturb, solve, perturb .... Every perturb calls factorize linear constraints twice (at the beginning and at the end). solve calls it once. If we zoom to one of them, it looks like this,
image

proximal-lsq-exact optimization

This is even more complicated to see, but here it is,
image

Here I zoom the second iteration,
image

Zooming even further,
image
I guess the the first to factorize_linear_constraints call is from perturb, and the other one that is under LinearConstraintProjection is for eq.solve.

@YigitElma
Copy link
Collaborator

YigitElma commented Nov 21, 2024

Btw, I have noticed that perturb still uses SVD and tr_step_exact_svd. Was that deliberate?

The second factorization in perturbation can be made faster since Z is the same but I don't know if it is easy or worth working on. D and b should change, so the new method might end up looking like the same function but with a faster particular solution finder without Z finder. I think #1374 should be enough for this.

I am not too familiar with that portion of the code, so sorry if there is any mistake.

@f0uriest
Copy link
Member

In perturb and in ProximalProjection we need to call factorize_linear_constraints multiple times because the vector b is changing, so we need to update the particular solution. If we stored Ainv (maybe as part of the project/recover operators) we could get around that.

@YigitElma
Copy link
Collaborator

Something like,

(
    xp,
    A,
    b,
    Z,
    D,
    unfixed_idx,
    project,
    recover,
    Ainv,
    A_full_nondegenerate,
    degenerate_idx, # maybe we need those for b_new
) = factorize_linear_constraints(
    objective,
    constraint,
)
b_new = -constraint.compute_scaled_error(0)
b_new = np.delete(b_new, degenerate_idx)
xp_new = jnp.zeros_like(xp)
fixed_idx = np.setdiff1d(np.arange(xp.size), unfixed_idx)
xp_new[fixed_idx] = b_new[fixed_idx]
xp_new[unfixed_idx] = Ainv @ (b_new-A_full_nondegenerate[:,fixed_idx] @ xp_new[fixed_idx])
recover = lambda x: xp_new + Z @ x

@dpanici dpanici added the P3 Highest Priority, someone is/should be actively working on this label Nov 25, 2024
@YigitElma
Copy link
Collaborator

Maybe in the build method of ProximalProjection, we can create and build a LinearConstraintProjection and use it for the rest. We also need to add a update_constraint_target method to LinearConstraintProjection such that ForceBalance objective, self-consistency constraints and null-space etc stays the same, but the target for the FixedParameter constraints and project/recover methods of the LinearConstraintProjection get updated.

Then pass this LinearConstraintProjection to perturb and solve in update_equilibrium method (we need some type checking in those though), and only update the targets along the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Highest Priority, someone is/should be actively working on this performance New feature or request to make the code faster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants