Skip to content

Alternative implementation to hide the interface so that all samplers can use HyperbandPruner.#1196

Merged
hvy merged 23 commits intooptuna:masterfrom
HideakiImamura:refactor/hyperband/sampler-logic
May 14, 2020
Merged

Alternative implementation to hide the interface so that all samplers can use HyperbandPruner.#1196
hvy merged 23 commits intooptuna:masterfrom
HideakiImamura:refactor/hyperband/sampler-logic

Conversation

@HideakiImamura
Copy link
Copy Markdown
Member

Motivation

The current Hyperband implementation in Optuna imposes users who want to implement a new sampler to implement the specific logic. This annoys us because it is exactly the same logic for all samplers. To hide the interface so that all samplers can use HyperbandPruner without any additional efforts, PR #1168 which is already opened needs to fix the interface of study.sampler. This PR aims to provide an alternative choice to implement the functionality without any changes in interfaces.

Description of the changes

  • Move sampler specific logic in optuna/samplers/tpe/sampler.py to pruners/__init__.py as pruners.filter_study() function.
  • Add study filtering logic to optuna/trial.py.
  • To avoid circular reference, make optuna/_trial_state.py and move TrialState class from optuna/trial.py to optuna/_trial_state.py.

Note

This PR opposite PR #1168. If PR #1168 is merged, this PR should not be merged, and vice versa.

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 1, 2020

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.97%. Comparing base (b1197ee) to head (b9b4612).
Report is 13195 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
+ Coverage   85.93%   85.97%   +0.03%     
==========================================
  Files          92       92              
  Lines        6747     6751       +4     
==========================================
+ Hits         5798     5804       +6     
+ Misses        949      947       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hvy hvy self-assigned this May 7, 2020
@hvy hvy added enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. optuna.pruners Related to the `optuna.pruners` submodule. This is automatically labeled by github-actions. labels May 8, 2020
Copy link
Copy Markdown
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I left some comments. Also, I think we can test this logic, it might not be the cleanest approach but you could e.g. patch _get_bracket_id and count the number of trials.

@hvy
Copy link
Copy Markdown
Member

hvy commented May 13, 2020

How about testing that filtering works for each sampler?

@pytest.mark.parametrize(
    "sampler_cls,sampler_kwargs",
    [
        (optuna.samplers.RandomSampler, {}),
        (optuna.samplers.TPESampler, {"n_startup_trials": 1}),
        (
            optuna.samplers.GridSampler,
            {"search_space": {"value": numpy.linspace(0.0, 1.0, 10, endpoint=False).tolist()}},
        ),
        (optuna.samplers.CmaEsSampler, {"n_startup_trials": 1}),
    ],
)
def test_hyperband_filter_study(sampler_cls, sampler_kwargs):
    # type: () -> None
    def objective(trial: optuna.trial.Trial) -> float:
        return trial.suggest_uniform("value", 0.0, 1.0)
    n_trials = 10
    n_brackets = 2
    expected_n_trials_per_bracket = n_trials // n_brackets
    with mock.patch(
        "optuna.pruners.HyperbandPruner._get_bracket_id",
        new=mock.Mock(side_effect=lambda study, trial: trial.number % n_brackets),
    ):
        for method_name in [
            "infer_relative_search_space",
            "sample_relative",
            "sample_independent",
        ]:
            sampler = sampler_cls(**sampler_kwargs)
            pruner = optuna.pruners.HyperbandPruner(
                min_resource=MIN_RESOURCE,
                max_resource=MAX_RESOURCE,
                reduction_factor=REDUCTION_FACTOR,
            )
            with mock.patch(
                "optuna.samplers.{}.{}".format(sampler_cls.__name__, method_name),
                wraps=getattr(sampler, method_name),
            ) as method_mock:
                study = optuna.study.create_study(sampler=sampler, pruner=pruner)
                study.optimize(objective, n_trials=n_trials)
                args = method_mock.call_args[0]
                study = args[0]
                trials = study.get_trials()
                assert len(trials) == expected_n_trials_per_bracket

Copy link
Copy Markdown
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your update. I have two comments.

[Note] #1210 also addresses the refactoring of TrialState. It may change the module of TrialState.

@HideakiImamura
Copy link
Copy Markdown
Member Author

@toshihikoyanase Thank you for your insightful comments. I updated codes according to your comments.

[Note] #1210 also addresses the refactoring of TrialState. It may change the module of TrialState.

Thank you for the information. I will carefully merge this PR and #1210.

@toshihikoyanase
Copy link
Copy Markdown
Member

@HideakiImamura Thank you for reflecting my suggestions!

I have an additional comment.
As the PR title says, we only apply _filter_study to the methods related to samplers. Currently, we don't use it in Trial.should_prune because HyperbandPruner filters trials in the prune method.

I think it is worth mentioning it somewhere in the code for future development. For instance, how about adding a test case to confirm that _filter_study is not applied in Trial.should_prune?

Copy link
Copy Markdown
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! Changes basically LGTM but I left some nitpicks.

@hvy hvy added this to the v1.5.0 milestone May 13, 2020
@hvy
Copy link
Copy Markdown
Member

hvy commented May 14, 2020

Could you fix the conflict as well?

@HideakiImamura
Copy link
Copy Markdown
Member Author

@hvy @toshihikoyanase Thank you for your reviews. I updated according to your comments.

Copy link
Copy Markdown
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your update. LGTM!

@hvy
Copy link
Copy Markdown
Member

hvy commented May 14, 2020

Ouch, another conflict.

Copy link
Copy Markdown
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hvy hvy merged commit 0b6e303 into optuna:master May 14, 2020
@hvy hvy changed the title Altenative implementation to hide the interface so that all samplers can use HyperbandPruner Altenative implementation to hide the interface so that all samplers can use HyperbandPruner. May 14, 2020
@hvy
Copy link
Copy Markdown
Member

hvy commented May 14, 2020

Thanks for the long running effort. The CI was all green so I merged your changes.

@hvy hvy changed the title Altenative implementation to hide the interface so that all samplers can use HyperbandPruner. Alternative implementation to hide the interface so that all samplers can use HyperbandPruner. May 29, 2020
@HideakiImamura HideakiImamura deleted the refactor/hyperband/sampler-logic branch May 18, 2021 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. optuna.pruners Related to the `optuna.pruners` submodule. This is automatically labeled by github-actions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants