Skip to content

Conversation

@szd5654125
Copy link

Summary

This PR fixes a bug where passing stopping_criterion=None to
convergence_plot / process_benchmark_results raises an
UnboundLocalError, although the API explicitly documents None
as a valid option.

Changes

  • Handle stopping_criterion=None explicitly in _process_one_result
    and ensure is_converged is always defined.
  • Ensure histories are not truncated when no stopping criterion is applied.
  • Update tests to reflect the documented behavior.

Related issue

Closes #648

@codecov
Copy link

codecov bot commented Dec 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/optimagic/visualization/convergence_plot.py 95.65% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@r3kste r3kste left a comment

Choose a reason for hiding this comment

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

After looking through this, I am unsure whether stopping_criterion is actually allowed to be None. On one hand, the docstring of process_benchmark_results mentions that passing None is valid, although the logic for handling it was not present. On the other hand, profile_plot explicitly raises an error in this case.

Personally, I think that the docstring was wrong/outdated, and passing None to stopping_criterion was never supported. This change was made in commit 68d941, but it is not clear why.

@janosg It would be great if you could clarify whether stopping_criterion can be None, and if so, what the expected behavior should be.

@janosg
Copy link
Member

janosg commented Dec 19, 2025

Thanks @r3kste and @szd5654125.

I think the None case should actually not be supported. If someone wanted to disable all convergence criteria, they could set the precisions to extremely small values or even 0.

Could we change this PR to improve the type hints and documentation for this?

@szd5654125
Copy link
Author

I agree that stopping_criterion=None should not be supported.
Given this, I can update this PR to:
clarify the documentation,
Restore the previous test cases,

I'll push an update shortly.

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.

UnboundLocalError when stopping_criterion = None in convergence_plot().

3 participants