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

Code quality: Shadow variable declaration in l1_proximal.h #378

Open
krishnakumarg1984 opened this issue Jun 22, 2023 · 2 comments · May be fixed by #412
Open

Code quality: Shadow variable declaration in l1_proximal.h #378

krishnakumarg1984 opened this issue Jun 22, 2023 · 2 comments · May be fixed by #412

Comments

@krishnakumarg1984
Copy link
Collaborator

Variable res is declared here, and then shadowed within the loop again here.

Vector<Scalar> const res = Psi().adjoint() * out;   // outer variable declaration here
  Vector<Scalar> u_l1 = 1e0 / nu() * (res - apply_soft_threshhold(gamma, res));
  apply_constraints(out, x - Psi() * u_l1);

  // Move on to other iterations
  for (++niters; niters < itermax() or itermax() == 0; ++niters) {
    auto const do_break = breaker(objective(x, out, gamma));
    SOPT_LOW_LOG("    - [ProxL1] iter {}, prox_fval = {}, rel_fval = {}", niters, breaker.current(),
                 breaker.relative_variation());
    if (do_break) break;

    Vector<Scalar> const res = u_l1 * nu() + Psi().adjoint() * out;  // shadow variable declaration here
@SJaffa
Copy link
Contributor

SJaffa commented Jul 4, 2023

It looks like res is not used after the loop or returned from the function so could we rename the outer declaration to initial_res or something similar?

@krishnakumarg1984
Copy link
Collaborator Author

@SJaffa It's unclear to me what this function is doing. Could you please see if the rename breaks the building?

@SJaffa SJaffa linked a pull request Jul 4, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants