Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TPE discrete-categorical-loguniform space support #389

Merged
merged 11 commits into from
Jun 12, 2020

Conversation

donglinjy
Copy link
Collaborator

Add discrete and categorical dimension into TPE support.

Now TPE will support uniform, uniform discrete and choices as prior. As for choices prior, the probabilities if any given will be ignored.

@codecov-io
Copy link

codecov-io commented May 8, 2020

Codecov Report

Merging #389 into develop will increase coverage by 0.62%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #389      +/-   ##
===========================================
+ Coverage    46.61%   47.23%   +0.62%     
===========================================
  Files           67       67              
  Lines        12754    12905     +151     
  Branches       317      321       +4     
===========================================
+ Hits          5945     6096     +151     
  Misses        6784     6784              
  Partials        25       25              
Impacted Files Coverage Δ
tests/functional/algos/test_algos.py 100.00% <100.00%> (ø)
tests/unittests/algo/test_tpe.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1057ea...7bc6808. Read the comment docs.

if list(candidate_points):
sampler_above = CategoricalSampler(self, above_points, choices)

lik_blow = sampler_below.get_loglikelis(candidate_points)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lik_blow = sampler_below.get_loglikelis(candidate_points)
lik_below = sampler_below.get_loglikelis(candidate_points)

lik_blow = sampler_below.get_loglikelis(candidate_points)
lik_above = sampler_above.get_loglikelis(candidate_points)

new_point = compute_max_ei_point(candidate_points, lik_blow, lik_above)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
new_point = compute_max_ei_point(candidate_points, lik_blow, lik_above)
new_point = compute_max_ei_point(candidate_points, lik_below, lik_above)

if list(candidate_points):
sampler_above = CategoricalSampler(self, above_points, choices)

lik_blow = sampler_below.get_loglikelis(candidate_points)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lik_blow = sampler_below.get_loglikelis(candidate_points)
lik_below = sampler_below.get_loglikelis(candidate_points)

lik_blow = sampler_below.get_loglikelis(candidate_points)
lik_above = sampler_above.get_loglikelis(candidate_points)

new_point_index = compute_max_ei_point(candidate_points, lik_blow, lik_above)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
new_point_index = compute_max_ei_point(candidate_points, lik_blow, lik_above)
new_point_index = compute_max_ei_point(candidate_points, lik_below, lik_above)

@@ -262,13 +370,13 @@ def test_set_state(self, tpe):
def test_unsupported_space(self):
"""Test tpe only work for supported search space"""
space = Space()
dim = Integer('yolo1', 'uniform', -2, 4)
dim = Fidelity('epoch', 1, 9, 3)
Copy link
Member

Choose a reason for hiding this comment

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

It could support easily Fidelity by just returning the max like random search does. Integer('yolo1', 'normal', 0, 1) for testing unsupported priors.

Copy link
Collaborator Author

@donglinjy donglinjy May 12, 2020

Choose a reason for hiding this comment

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

It is pretty straight forward for random as all suggestions simply follow the same pattern, for tpe, we need to add separate paths to handle fidelity dimension and mention such support in doc. The only value for random or tpe to support fidelity I can think is to make it easier for user to switch from different search algorithms, or user can simply specify fixed fidelity value for random and tpe. And it is actually a little bit confusing as fidelity prior means differently between tpe/random and hyperband/asha,

Copy link
Member

Choose a reason for hiding this comment

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

Yes the advantage of supporting fidelity dimension in algos like random search and TPE is really just to make it easier to switch from one algo to another using the same search space definition.

above_points = [obs_points[25:]]
points = tpe.sample_one_dimension(dim1, 1,
below_points, above_points, tpe._sample_int_point)
assert len(points) == 1
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to test the values as well. That they are integers in the correct interval and correspond roughly to the best region given below_points and above_points.

assert len(points) == 2

tpe.n_ei_candidates = 0
points = tpe.sample_real_dimension(dim2, 2, below_points, above_points)
points = tpe.sample_one_dimension(dim2, 2,
below_points, above_points, tpe._sample_real_point)
Copy link
Member

Choose a reason for hiding this comment

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

I see that we didn't test for values for test_sample_real_dimensions. We could sample separately below and above with different intervals to artificially create the best regions and verify the sampling of the TPE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was same idea for int, categorical and real, as there are separate cases for gmm and categorical sampler. But sure, we can add some specific verification at these places.

@donglinjy donglinjy changed the title TPE discrete categorical space support TPE discrete-categorical-loguniform space support May 23, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2020

Codecov Report

Merging #389 into develop will increase coverage by 0.67%.
The diff coverage is 99.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #389      +/-   ##
===========================================
+ Coverage    46.62%   47.30%   +0.67%     
===========================================
  Files           67       67              
  Lines        12768    12932     +164     
  Branches       317      321       +4     
===========================================
+ Hits          5953     6117     +164     
  Misses        6790     6790              
  Partials        25       25              
Impacted Files Coverage Δ
tests/unittests/core/test_transformer.py 21.66% <0.00%> (ø)
tests/functional/algos/test_algos.py 100.00% <100.00%> (ø)
tests/unittests/algo/test_space.py 99.27% <100.00%> (-0.01%) ⬇️
tests/unittests/algo/test_tpe.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b956ec...30db153. Read the comment docs.

src/orion/algo/tpe.py Outdated Show resolved Hide resolved
@@ -283,7 +280,8 @@ def suggest(self, num=1):
above_points[idx: idx + shape[0]],
self._sample_categorical_point)
else:
raise NotImplementedError()
# fidelity dimension
Copy link
Member

Choose a reason for hiding this comment

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

maybe elif dimension.type == 'fidelity' to be sure to catch unsupported dimensions that would have been missed in __init__

@Thomsch Thomsch merged commit 047aee5 into Epistimio:develop Jun 12, 2020
This was referenced Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants