Repository: optuna/optuna
Pull request: Trial level suggest for same variable with different parameters give warning
PR for #797
and fix for #867

PR is not completed yet.

- [x] more tests

Problem statement:
Trial suggest for same variable with different parameters should give error or raise exceptoion
## Motivation
It is allowed to call `trial.suggest` multiple times for the same variable (`x` in this example) (see below). This always gives the same value (see assert below).

```Python
def objective(trial):
    x = trial.suggest_uniform('x', 0, 10)
    print(x)
    y = trial.suggest_uniform('x', 0, 10)
    print(y)
    assert x == y
    return (x - 2) ** 2
```

This is good because x might be needed multiple times in a loop (for cross validation for example).

But this here is also possible:

```Python
def objective(trial):
    x = trial.suggest_uniform('x', 0, 10)
    print(x)
    y = trial.suggest_uniform('x', -20, -10)
    print(y)
    assert x == y # assert is still true here
    return (x - 2) ** 2
```

This should not be allowed since `trial.suggest_uniform('x', 0, 10)` is in contradiction to `trial.suggest_uniform('x', -20, -10)`. The current implementation gives the same for y as the value for x was.

## Description

IMO this situation where one "suggest" is in contradiction to an other for the same variable should give a warning or raise an exception. This is because it is a hint for a programming error that might not get noticed otherwise.

What do you think? After a discussion I might contribute the change.

Hints from issue discussion:
Hi,

Thank you for your proposal.
I think your opinion is very reasonable, so I'd appreciate it if you could create a PR to address this issue.

> should give a warning or raise an exception.

Personally, I prefer a warning because it doesn't break backward compatibility (but it might be needed to discuss this choice with you and other developers on PR).
Ok great. I will work on this.
Thanks! 
Started the PR here: ~~#867~~ #908

Commit message: no default value for name param

Patch to review:
diff --git a/optuna/trial.py b/optuna/trial.py
index ba8eef87949..8b53e082c7c 100644
--- a/optuna/trial.py
+++ b/optuna/trial.py
@@ -136,6 +136,8 @@ def __init__(
 
         self._init_relative_params()
 
+        self.distributions_in_trial = {}  # type: Dict[str, Any]
+
     def _init_relative_params(self):
         # type: () -> None
 
@@ -178,6 +180,8 @@ def suggest_uniform(self, name, low, high):
             A suggested float value.
         """
 
+        self._check_distribution(low=low, high=high, name=name)
+
         distribution = distributions.UniformDistribution(low=low, high=high)
         if low == high:
             return self._set_new_param_or_get_existing(name, low, distribution)
@@ -218,6 +222,8 @@ def suggest_loguniform(self, name, low, high):
             A suggested float value.
         """
 
+        self._check_distribution(low=low, high=high, name=name)
+
         distribution = distributions.LogUniformDistribution(low=low, high=high)
         if low == high:
             return self._set_new_param_or_get_existing(name, low, distribution)
@@ -265,6 +271,8 @@ def suggest_discrete_uniform(self, name, low, high, q):
             A suggested float value.
         """
 
+        self._check_distribution(low=low, high=high, q=q, name=name)
+
         high = _adjust_discrete_uniform_high(name, low, high, q)
         distribution = distributions.DiscreteUniformDistribution(low=low, high=high, q=q)
         if low == high:
@@ -303,6 +311,8 @@ def suggest_int(self, name, low, high):
             A suggested integer value.
         """
 
+        self._check_distribution(low=low, high=high, name=name)
+
         distribution = distributions.IntUniformDistribution(low=low, high=high)
         if low == high:
             return self._set_new_param_or_get_existing(name, low, distribution)
@@ -342,6 +352,10 @@ def suggest_categorical(self, name, choices):
         """
 
         choices = tuple(choices)
+
+        # no deed to call self._check_distribution because
+        # CategoricalDistribution does not support dynamic value space
+
         return self._suggest(name, distributions.CategoricalDistribution(choices=choices))
 
     def report(self, value, step=None):
@@ -516,6 +530,26 @@ def _is_relative_param(self, name, distribution):
         param_value_in_internal_repr = distribution.to_internal_repr(param_value)
         return distribution._contains(param_value_in_internal_repr)
 
+    def _check_distribution(self, name, low=None, high=None, q=None):
+        # type: (str, Any, Any, Optional[float]) -> None
+
+        dist_dict = {}
+        if low is not None:
+            dist_dict['low'] = low
+        if high is not None:
+            dist_dict['high'] = high
+        if q is not None:
+            dist_dict['q'] = q
+
+        old_distribution_in_trial = self.distributions_in_trial.get(name, None)
+
+        if old_distribution_in_trial is None:
+            self.distributions_in_trial[name] = dist_dict
+        elif old_distribution_in_trial != dist_dict:
+            warnings.warn('Inconsistent parameter values for distribution {}! '
+                          'This might be a configuration mistake.'
+                          .format(name), RuntimeWarning)
+
     @property
     def number(self):
         # type: () -> int
diff --git a/tests/test_trial.py b/tests/test_trial.py
index 04746a1f588..469bae0385d 100644
--- a/tests/test_trial.py
+++ b/tests/test_trial.py
@@ -25,6 +25,74 @@
     [storages.InMemoryStorage, lambda: storages.RDBStorage('sqlite:///:memory:')])
 
 
+@parametrize_storage
+def test_check_distribution_suggest_uniform(storage_init_func):
+    # type: (typing.Callable[[], storages.BaseStorage]) -> None
+
+    sampler = samplers.RandomSampler()
+    study = create_study(storage_init_func(), sampler=sampler)
+    trial = Trial(study, study._storage.create_new_trial(study._study_id))
+
+    with pytest.warns(None) as record:
+        trial.suggest_uniform('x', 10, 20)
+        trial.suggest_uniform('x', 10, 20)
+        trial.suggest_uniform('x', 10, 30)
+
+    # we expect exactly one warning
+    assert len(record) == 1
+
+
+@parametrize_storage
+def test_check_distribution_suggest_loguniform(storage_init_func):
+    # type: (typing.Callable[[], storages.BaseStorage]) -> None
+
+    sampler = samplers.RandomSampler()
+    study = create_study(storage_init_func(), sampler=sampler)
+    trial = Trial(study, study._storage.create_new_trial(study._study_id))
+
+    with pytest.warns(None) as record:
+        trial.suggest_loguniform('x', 10, 20)
+        trial.suggest_loguniform('x', 10, 20)
+        trial.suggest_loguniform('x', 10, 30)
+
+    # we expect exactly one warning
+    assert len(record) == 1
+
+
+@parametrize_storage
+def test_check_distribution_suggest_discrete_uniform(storage_init_func):
+    # type: (typing.Callable[[], storages.BaseStorage]) -> None
+
+    sampler = samplers.RandomSampler()
+    study = create_study(storage_init_func(), sampler=sampler)
+    trial = Trial(study, study._storage.create_new_trial(study._study_id))
+
+    with pytest.warns(None) as record:
+        trial.suggest_discrete_uniform('x', 10, 20, 2)
+        trial.suggest_discrete_uniform('x', 10, 20, 2)
+        trial.suggest_discrete_uniform('x', 10, 22, 2)
+
+    # we expect exactly one warning
+    assert len(record) == 1
+
+
+@parametrize_storage
+def test_check_distribution_suggest_int(storage_init_func):
+    # type: (typing.Callable[[], storages.BaseStorage]) -> None
+
+    sampler = samplers.RandomSampler()
+    study = create_study(storage_init_func(), sampler=sampler)
+    trial = Trial(study, study._storage.create_new_trial(study._study_id))
+
+    with pytest.warns(None) as record:
+        trial.suggest_int('x', 10, 20)
+        trial.suggest_int('x', 10, 20)
+        trial.suggest_int('x', 10, 22)
+
+    # we expect exactly one warning
+    assert len(record) == 1
+
+
 @parametrize_storage
 def test_suggest_uniform(storage_init_func):
     # type: (typing.Callable[[], storages.BaseStorage]) -> None


Code review comments:
diff_hunk: @@ -342,6 +352,10 @@ def suggest_categorical(self, name, choices):
         """
 
         choices = tuple(choices)
+
+        # no deed to call self._check_distribution because
path: optuna/trial.py
text: @user1:
I believe that the `ValueError` raised with the categorical distribution is a slightly different topic (due to implementation details). While I'd like consistent behavior for the categorical distribution, the risk of performance overhead might not justify it. Could you just fix the sentence (and end it with a punctuation)?
```suggestion
        # No need to call self._check_distribution because
```

@author:
fixed

diff_hunk: @@ -136,6 +136,8 @@ def __init__(
 
         self._init_relative_params()
 
+        self.distributions_in_trial = {}  # type: Dict[str, Any]
path: optuna/trial.py
text: @user1:
You could be more precise.
```suggestion
        self.distributions_in_trial = {}  # type: Dict[str, Dict[str, Any]]
```

diff_hunk: @@ -136,6 +136,8 @@ def __init__(
 
         self._init_relative_params()
 
+        self.distributions_in_trial = {}  # type: Dict[str, Any]
path: optuna/trial.py
text: @user1:
Could you prefix the attribute with an underscore as it is not meant to be a part of the public API?
```suggestion
        self._distributions_in_trial = {}  # type: Dict[str, Any]
```

@author:
Fixed