Warn when GPSampler falls back to RandomSampler#6179
Warn when GPSampler falls back to RandomSampler#6179y0z merged 1 commit intooptuna:masterfrom sisird864:warn-gp-sampler-on-fallback
GPSampler falls back to RandomSampler#6179Conversation
|
@y0z Could you review this PR? |
|
@fusawa-yugo |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a runtime warning to inform users when GPSampler falls back to RandomSampler for independent sampling. Key changes include importing and initializing a module-level logger, introducing the warn_independent_sampling parameter with corresponding logging helper, and integrating the warning log in sample_relative and sample_independent methods.
|
Thanks for the feedback. I added these changes:
Pushed the update. Please have another look. |
|
Could you please review this PR? |
fusawa-yugo
left a comment
There was a problem hiding this comment.
Thank you for your PR!!
I left some comments. Could you check them?
optuna/samplers/_gp/sampler.py
Outdated
| if self._warn_independent_sampling: | ||
| self._log_independent_sampling(trial, param_name) |
There was a problem hiding this comment.
A warning should be displayed when sampling independently and the number of trials are sufficient for the same reason as previous comment
| if self._warn_independent_sampling: | |
| self._log_independent_sampling(trial, param_name) | |
| if self._warn_independent_sampling: | |
| states = (TrialState.COMPLETE,) | |
| complete_trials = study._get_trials(deepcopy=False, states=states, use_cache=True) | |
| if len(complete_trials) >= self._n_startup_trials: | |
| self._log_independent_sampling(trial, param_name) |
There was a problem hiding this comment.
@sisird864
Could you please apply the suggestion?
Once the change is applied, this PR looks good to me:)
There was a problem hiding this comment.
Hello, I’ve added the startup-threshold guard to sample_independent as requested pushed the update. CI is green now. Please take another look. Thanks.
|
Thanks for the review, I’ve updated the PR to:
Please have another look. Thank you. |
nabenabe0928
left a comment
There was a problem hiding this comment.
Hey thank you for the PR, I left some comments:)
|
@nabenabe0928 I agree that repeated logging does not harm. Especially when the code regarding the search space gets more complex, debugging it becomes much more challenging and some support to the users could be great. I wouldn't mind the extra message in my log files, either. |
|
Thanks for the discussion. @1kastner makes a great point. Repeated warnings can help users debug unexpected independent sampling. I’ll remove the one-time disable in |
|
This PR relates to: |
GPSampler falls back to RandomSampler
nabenabe0928
left a comment
There was a problem hiding this comment.
Thank you for the changes, LGTM")
|
LGTM!! |
Motivation
Add a runtime warning to GPSampler when it falls back to RandomSampler, per the community Q&A suggestion. This informs users whenever the Gaussian-process sampler degrades to independent sampling (e.g. due to dynamic search-space changes or GP fitting errors) and keeps its behavior consistent with CmaEsSampler.
Description of the changes
Imports & logger
Warning helper
Guarded fallback
Modified the except Exception block in sample_independent to:
Testing
No new unit tests were added for the log message itself (logging was manually smoke-tested per maintainer guidance).