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

feat: Report processor switches to osqp when highs solver fails to converge. #1976

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ple13
Copy link
Contributor

@ple13 ple13 commented Dec 18, 2024

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

@ple13 ple13 requested a review from kungfucraig December 18, 2024 21:38
Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have some tests.

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ple13)


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 155 at r2 (raw file):

  def _solve_with_initial_value(self, solver_name, x0) -> Solution:
    problem = self._problem()
    solution = solve_problem(problem, solver=solver_name, initvals=x0, verbose=False)

nit: Maybe just write out "initial_values"

Otherwise at least put an underscore between the words.


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 158 at r2 (raw file):

    return solution

  def _problem(self):

While you're in here you may as well add some logging about each attempt.


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 189 at r2 (raw file):

          attempt_count += 1

      # If the highs solver does not converge, switch to the osqp solver which

So solve with HIGHS for 10 attempts and if those fail, try just once with OSPQ?

Copy link
Contributor Author

@ple13 ple13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a unit test to verify that osqp will be executed when highs does not converge.

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @kungfucraig)


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 155 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

nit: Maybe just write out "initial_values"

Otherwise at least put an underscore between the words.

Done.


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 158 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

While you're in here you may as well add some logging about each attempt.

I have the PR#1878 to handle the log. I'll have the logs included in that PR (after this PR has been merged). WDYT?


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 189 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

So solve with HIGHS for 10 attempts and if those fail, try just once with OSPQ?

Done.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @ple13)


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 189 at r2 (raw file):

Previously, ple13 (Phi) wrote…

Done.

Sorry. My comment was ambiguous.

Why try 10 times with the other one and just once with this one?

With respect to the comment you added, you noted it was more robust, so you should explain why we don't use it to start with.

Copy link
Contributor Author

@ple13 ple13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @kungfucraig)


src/main/python/wfa/measurement/reporting/postprocessing/noiseninja/solver.py line 189 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Sorry. My comment was ambiguous.

Why try 10 times with the other one and just once with this one?

With respect to the comment you added, you noted it was more robust, so you should explain why we don't use it to start with.

I'm sorry I didn't explain this better. The reason why I didn't try 10 times is because I thought the osqp solver was deterministic given the initial values. However, it is also a randomization process due to some non-deterministic internal states. I've added more comments to explain why we go for the highs solver first even though osqp is more robust (page 3 in https://arxiv.org/pdf/1711.08013 discusses first-order method (like osqp) vs others and suggests that osqp is less accurate.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants