Repository navigation
Batched strategies - #53
Merged
Merged
Conversation
vreshniak
marked this pull request as draft
August 6, 2026 14:42
Do not train surrogate at initialization when dataset_y is empty
`def _extract_y_train_from_dataset` implements cached_property logic
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and test-coverage regressions (e.g., believer uses a stale model during batch selection, missing validation can raise UnboundLocalError, and a duplicated test name silently drops coverage).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request adds support for batched point-selection strategies (constant liar and kriging believer) and introduces deterministic/indexed surrogate-free strategies (center/corners/grid/chebyshev/latin hypercube), alongside a few stability/correctness fixes in workflow initialization and preprocessing.
Changes:
- Added indexed strategy implementations and selection logic, plus batch sampling via liar/believer.
- Expanded request dataclasses to support
batch_strategyand richerstrategy_argstypes. - Improved serverside caching behavior and added extensive unit tests + a client script example.
File summaries
| File | Description |
|---|---|
| tests/unit/test_internals.py | Adds helper utilities and new unit tests for indexed and batched strategies. |
| src/dial_service/utilities/strategies.py | Implements indexed strategies and new batch_sampling for liar/believer. |
| src/dial_service/serverside_data.py | Refactors y/yerr extraction into cached properties and adds cache clearing utility; updates multiple-input params handling. |
| src/dial_service/dial_service.py | Prevents model training at workflow init unless both X and non-empty Y are provided. |
| src/dial_service/core.py | Routes indexed single-point strategies and delegates batch requests to batch_sampling. |
| src/dial_dataclass/dial_dataclass.py | Extends strategy typing and adds batch_strategy to multi-point requests. |
| scripts/2d_rosenbrock_client_with_strategies.py | Adds an example client showcasing strategy scheduling and plotting. |
Review details
Suppressed comments (1)
src/dial_service/core.py:107
get_next_pointscomputesbackend/moduletwice (once before the batch-strategy check and again after thematch). This is redundant and makes it easier for the two blocks to drift if later edits change one but not the other.
backend = data.backend.lower()
module = get_backend_module(backend)
if data.batch_strategy is not None:
return batch_sampling(module, model, data)
match data.strategy:
case 'random':
output_points = [
random_in_bounds(data.bounds, data.numpy_rng) for _ in range(data.points)
]
return output_points
case 'hypercube':
output_points = hypercube(data.bounds, data.points, data.numpy_rng)
return output_points
backend = data.backend.lower()
module = get_backend_module(backend)
output_points = module.samples(module, model, data)
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+80
to
+98
| @cached_property | ||
| def yerr_train_raw(self) -> any: | ||
| """ | ||
| Return the raw training error values extracted from the dataset. | ||
| """ | ||
| yerr_label = self.statistics_y.scale | ||
| if isinstance(yerr_label, float): | ||
| self.yerr_train_raw = yerr_label | ||
| _yerr_train_raw = yerr_label | ||
| else: | ||
| # yerr_label is str | ||
| # this may trigger a ValueError, but should be handled by dataclass validation | ||
| pos_yerr = self.labels_y.index(yerr_label) | ||
| self.yerr_train_raw = self.dataset_y[:, pos_yerr] | ||
| _yerr_train_raw = self.dataset_y[:, pos_yerr] | ||
|
|
||
| if np.any(self.yerr_train_raw < 0): | ||
| idxs = np.where(np.yerr_train_raw < 0) | ||
| msg = f'yerr values in statistics_y.scale must be non-negative, found {np.yerr_train_raw[idxs[0]]} at {idxs[0]}.' | ||
| if np.any(_yerr_train_raw < 0): | ||
| idxs = np.where(_yerr_train_raw < 0) | ||
| msg = f'yerr values in statistics_y.scale must be non-negative, found {_yerr_train_raw[idxs[0]]} at {idxs[0]}.' | ||
| raise ValueError(msg) | ||
| return _yerr_train_raw |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Lance-Drane <Lance-Drane@users.noreply.github.com>
Signed-off-by: Lance-Drane <Lance-Drane@users.noreply.github.com>
Lance-Drane
approved these changes
Sep 9, 2026
Lance-Drane
left a comment
Lance-Drane
left a comment
Contributor
There was a problem hiding this comment.
This LGTM, I went ahead and fixed the bug that CoPilot caught. Going to go ahead and merge this.
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.
This pull request introduces support of batch strategies.
Key changes include:
Strategy Handling
'liar','believer'). The strategy argument types were also expanded to allow lists for more flexible parameterization.ServersideInputMultipleOtherStrategyand related data classes to accept and process the new strategy types and arguments, including batch strategies.Core Logic
core.pyto support batch sampling. Now,get_next_pointscan handle the new strategies, delegating to the appropriate selection logic based on the strategy type.strategies.pyforbatch_samplingsupporting both liar and believer batch strategies.Fixes and Minor Improvements
initialize_workflowto only train a model if bothdataset_xand non-emptydataset_yare provided, preventing unnecessary or invalid model training.1e-12) in_transform_Y_params.