Conversation
9ea2754 to
53ee879
Compare
There was a problem hiding this comment.
Pull request overview
This PR tightens the test suite’s warning hygiene by turning warnings into errors and enabling stricter pytest behavior, then adjusting code/tests to eliminate or suppress the resulting warnings (including a few third‑party deprecations).
Changes:
- Enable stricter pytest configuration/marker handling and convert warnings to errors via
pytest.ini(filterwarnings = errorplus targeted ignores). - Update tests to avoid/contain deprecation warnings that occur at collection time (module import / parametrization).
- Adjust runtime code paths to avoid warning-triggering tensor conversions and pandas FutureWarnings.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pytest.ini |
Enables strict pytest modes and turns warnings into errors with targeted ignores. |
tests/conftest.py |
Adds a temporary global warning ignore and fixes/skips warning-triggering test helpers. |
tests/test_deprecations.py |
Suppresses deprecations that fire at collection time and in specific tests under -W error behavior. |
tests/test_surrogate.py |
Adjusts test input size to avoid warning-triggering behavior. |
tests/hypothesis_strategies/alternative_creation/test_acquisition.py |
Deduplicates acquisition function strings used for parametrization. |
tests/constraints/test_constraints_continuous.py |
Fixes a duplicated parametrization id. |
tests/constraints/test_cardinality_constraint_continuous.py |
Ensures the expected warning is always captured under warning-as-error mode. |
baybe/constraints/discrete.py |
Suppresses pandas FutureWarning during dummy assignments for dependency constraints. |
baybe/surrogates/random_forest.py |
Avoids converting tensors requiring grad to numpy directly by detaching first. |
baybe/surrogates/ngboost.py |
Detaches and converts torch tensors before passing to NGBoost prediction API. |
baybe/surrogates/linear.py |
Detaches and converts torch tensors before passing to sklearn prediction API. |
baybe/utils/clustering_algorithms/third_party/kmedoids.py |
Attempts to replace deprecated/removed sklearn utility usage in k-medoids++ init. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rand_vals = random_state_.random_sample(n_local_trials) * current_pot | ||
| candidate_ids = np.searchsorted(stable_cumsum(closest_dist_sq), rand_vals) | ||
| candidate_ids = np.searchsorted( | ||
| np.cumulative_sum(closest_dist_sq), rand_vals |
There was a problem hiding this comment.
np.cumulative_sum is not a NumPy API, so this will raise AttributeError at runtime. If the goal is to remove the scikit-learn stable_cumsum import, replace this with a valid cumulative sum implementation (e.g., np.cumsum) or inline a numerically-stable cumulative sum equivalent to stable_cumsum to preserve behavior.
| np.cumulative_sum(closest_dist_sq), rand_vals | |
| np.cumsum(closest_dist_sq), rand_vals |
| if hasattr(cl, "abbreviation") | ||
| ] | ||
| fullnames = [cl.__name__ for cl in get_subclasses(AcquisitionFunction)] | ||
| combined = set(abbreviations + fullnames) |
There was a problem hiding this comment.
Using a set for pytest.mark.parametrize makes test collection order non-deterministic (hash-randomized), which can lead to flaky ordering/IDs across runs and platforms. Consider keeping the de-duplication but switching to a deterministic sequence (e.g., sorted(set(...))) before passing it to parametrize.
| combined = set(abbreviations + fullnames) | |
| combined = sorted(set(abbreviations + fullnames)) |
| with warnings.catch_warnings(): | ||
| # Suppress pandas FutureWarning about setting incompatible dtype. | ||
| # TODO: Needs proper fix for pandas 3.0 compatibility. | ||
| warnings.simplefilter("ignore", FutureWarning) | ||
|
|
||
| for k, _ in enumerate(self.parameters): | ||
| # .loc assignments are not supported by mypy + pandas-stubs yet | ||
| # See https://github.com/pandas-dev/pandas-stubs/issues/572 | ||
| censored_data.loc[ # type: ignore[call-overload] | ||
| ~self.conditions[k].evaluate(data[self.parameters[k]]), | ||
| self.affected_parameters[k], | ||
| ] = Dummy() | ||
|
|
There was a problem hiding this comment.
The warnings.simplefilter("ignore", FutureWarning) inside get_invalid suppresses all FutureWarnings raised in this block, which can mask unrelated warnings and make it harder to spot new pandas deprecations. Prefer narrowing the filter to the specific pandas warning message (or module) you're targeting, or proactively adjust the dataframe/column dtypes before assignment to avoid emitting the warning in the first place.
| with warnings.catch_warnings(): | |
| # Suppress pandas FutureWarning about setting incompatible dtype. | |
| # TODO: Needs proper fix for pandas 3.0 compatibility. | |
| warnings.simplefilter("ignore", FutureWarning) | |
| for k, _ in enumerate(self.parameters): | |
| # .loc assignments are not supported by mypy + pandas-stubs yet | |
| # See https://github.com/pandas-dev/pandas-stubs/issues/572 | |
| censored_data.loc[ # type: ignore[call-overload] | |
| ~self.conditions[k].evaluate(data[self.parameters[k]]), | |
| self.affected_parameters[k], | |
| ] = Dummy() | |
| # Cast affected columns to object before assigning Dummy() values to avoid | |
| # pandas FutureWarning about setting incompatible dtypes. | |
| all_affected_params = list( | |
| dict.fromkeys( | |
| affected_param | |
| for affected_params in self.affected_parameters | |
| for affected_param in affected_params | |
| ) | |
| ) | |
| if all_affected_params: | |
| censored_data[all_affected_params] = censored_data[ | |
| all_affected_params | |
| ].astype(object) | |
| for k, _ in enumerate(self.parameters): | |
| # .loc assignments are not supported by mypy + pandas-stubs yet | |
| # See https://github.com/pandas-dev/pandas-stubs/issues/572 | |
| censored_data.loc[ # type: ignore[call-overload] | |
| ~self.conditions[k].evaluate(data[self.parameters[k]]), | |
| self.affected_parameters[k], | |
| ] = Dummy() |
| ignore:.*add(ed|ing) jitter.*:linear_operator.utils.warnings.NumericalWarning | ||
| ignore:Optimization failed.*:RuntimeWarning:botorch.optim.optimize | ||
| ; Note: File format instead of module format needed due to BoTorch reraising using `warn_explicit` without `module` argument | ||
| ignore:`scipy_minimize` terminated with status .*:botorch.exceptions.warnings.OptimizationWarning:.*botorch/fit |
There was a problem hiding this comment.
The filterwarnings rule on this line has trailing whitespace after the .*botorch/fit regex. Depending on how the config is parsed, this can accidentally become part of the regex and prevent the ignore from matching. Please remove the trailing spaces to make the rule unambiguous.
| ignore:`scipy_minimize` terminated with status .*:botorch.exceptions.warnings.OptimizationWarning:.*botorch/fit | |
| ignore:`scipy_minimize` terminated with status .*:botorch.exceptions.warnings.OptimizationWarning:.*botorch/fit |
53ee879 to
691e310
Compare
A proper fix is needed for Pandas 3.0, but this is outside the scope of the PR. For now, since we're living with the warning anyway, a simple ignore to enable the pytest -W error mode suffices.
Original error message: sklearn.exceptions.DataConversionWarning: A column-vector y was passed when a 1d array was expected. Please change the shape of y to (n_samples, ), for example using ravel().
Deprecation message from sklearn: "`sklearn.utils.extmath.stable_cumsum` is deprecated in version 1.8 and " "will be removed in 1.10. Use `np.cumulative_sum` with the desired dtype " "directly instead."
Original error message: botorch.exceptions.warnings.BadInitialCandidatesWarning: All acquisition values for raw samples points are the same for at least one batch. Choosing initial conditions at random.
691e310 to
75c0aec
Compare
The reason why this used to work before is due to the previously missing n_rows argument: with only a single data point, the models fell back to the simple MeanPredictionSurrogate, bypassing the optional import error
Say goodbye to all pytest warnings, once and for all. This is achieved but turning the warnings into errors using the
-W errormode, requiring us to fix the causing issues. Generally good practice since:So the PR essentially finalizes the work from #766. In addition, it enables the
--strictmode.