FIX: Fix thread-safety for adaptive_minmax#52
Merged
derb12 merged 4 commits intodevelopmentfrom Oct 21, 2025
Merged
Conversation
Some optimizer-type methods would behave differently when the algorithm_base was the _Optimizer class instead of Baseline[2D], namely under threaded tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes some issues with the testing framework when using threading, as found by using pytest-run-parallel. The majority of the identified test failures were just false-positives within the tests themselves since they were written without considering repeated, concurrent runs. The one true bug was that
adaptive_minmaxwas found to not be thread-safe in both 1D and 2D, and pointed to a flaw with how thetest_threadingtests worked for optimizer-type algorithms, as detailed below.Before this PR, the
BaseTesterclass used the underlying private categorical algorithm classes (eg._Whittaker,_Optimizer, etc.), rather than theBaselineclass to test each method. This is fine for most algorithms, and theBaseTester.test_threadingtest added by PR #33 correctly identified most of the thread-unsafe behavior within the Baseline and Baseline2D classes. However, for optimizer-type methods, this will not find any issues with threading because when getting the baseline method to use,_Optimizerswill always spawn a new object with the desired method (eg._Polynomialif method is 'modpoly') since they do not have those methods, which will always be thread-safe. In actual use, theBaselineclass will always have the desired method and will not spawn a new object. By changing theBaseTester[2D]test classes to useBaseline[2D]instead of the private classes, thetest_threadingtest then correctly failed foradaptive_minmaxin both the 1D and 2D cases, even without having to use pytest-run-parallel.This oversight in the testing framework was fixed, as well as the underlying threading issues for adaptive_minmax for both 1D and 2D.
Note that linting is expected to fail, those fixes are already in the optimize_lam branch.
Type of Pull Request
Pull Request Checklist
if applicable.