Add tests for SMAC #206
Add tests for SMAC #206
Conversation
| @@ -344,5 +353,5 @@ def _convert_to_config_space_design_space( | |||
| else: | |||
| raise NotImplementedError(f"Unknown Hyperparameter Type: {type(distribution)}") | |||
| if hp is not None: | |||
| config_space.add_hyperparameter(hp) | |||
| config_space.add(hp) | |||
There was a problem hiding this comment.
config_space.add_hyperparameter is deprecated and it is recommended to use add.
| @@ -237,6 +243,9 @@ def sample_relative( | |||
| study._storage.set_trial_system_attr(trial._trial_id, _SMAC_SEED_KEY, trial_info.seed) | |||
| params = {} | |||
| for name, hp_value in cfg.items(): | |||
| # SMAC uses np.int64 for integer parameters | |||
| if isinstance(hp_value, np.int64): | |||
| hp_value = hp_value.item() # Convert to Python int. | |||
There was a problem hiding this comment.
SMAC returns np.int64 by default, and it fails some tests. This change fixes them.
| @@ -226,6 +227,11 @@ def _get_init_design( | |||
| raise NotImplementedError(f"Unknown Initial Design Type: {init_design_type}") | |||
| return init_design | |||
|
|
|||
| def reseed_rng(self) -> None: | |||
There was a problem hiding this comment.
reseed_rng is currently not implemented. This will cause unexpected behavior when n_jobs > 1.
|
@dengdifan |
|
@nabenabe0928 Could you review this PR? |
There was a problem hiding this comment.
Thank you for the PR, I left a comment below!
Why are they not tested?
- test_sample_relative_categorical
- test_sample_single_distribution
- test_single_parameter_objective
- test_combination_of_different_distributions_objective
Could you add comment lines for the omits?
- test_reproducible_in_other_process
- test_reproducible
- test_sampler_reseed_rng
Can we maybe raise an error or warn in this scenario instead of skipping them?
- test_nan_objective_value
- test_dynamic_range_objective
- test_conditional_parameter_objective
| @@ -237,6 +243,9 @@ def sample_relative( | |||
| study._storage.set_trial_system_attr(trial._trial_id, _SMAC_SEED_KEY, trial_info.seed) | |||
| params = {} | |||
| for name, hp_value in cfg.items(): | |||
| # SMAC uses np.int64 for integer parameters | |||
| if isinstance(hp_value, np.int64): | |||
There was a problem hiding this comment.
| if isinstance(hp_value, np.int64): | |
| if isinstance(hp_value, np.integer): |
Contributor Agreements
Please read the contributor agreements and if you agree, please click the checkbox below.
Motivation
Add tests for SMAC to ensure its quality.
Description of the changes
Notes
Below is a summary of the features missing from
SMACSamplerthat I noticed while introducing the tests (and I skipped the tests for such features at the moment).reseed_rng(importance: relatively high)n_jobswill not work as expected sincestudy.optimizeinternally calls this method (cf. https://github.com/optuna/optuna/blob/467cca671e4940145f2feffe173dfcfc153b5744/optuna/study/_optimize.py#L102-L138). However, supporting this method seems non-trivial since SMAC does not support reseeding. As a temporary workaround, I added a warning to inform users thatreseed_rngdoes not work.