Fix #1018 (#1034)

* Fix #1018

* Test configspace conversion with kwargs

* Bound the number of positional argument for configspace conversion

* Update error message

* -

* -

Co-authored-by: Pierre Delaunay <[email protected]>

src/orion/algo/space/__init__.py

@@ -205,8 +205,8 @@ def validate(self):
             and self.default_value not in self
         ):
             raise ValueError(
-                "{} is not a valid value for this Dimension. "
-                "Can't set default value.".format(self.default_value)
+                "{} is not a valid value for dimension: {}, "
+                "Can't set default value.".format(self.default_value, self.name)
             )
 
     def _get_hashable_members(self):

src/orion/algo/space/configspace.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+from copy import deepcopy
 from functools import singledispatch
 from math import log10
 
@@ -57,6 +58,39 @@ def _qantization(dim: Dimension) -> float:
     return None
 
 
+def _upsert(array, i, value):
+    cp = len(array) - i
+
+    if value is None:
+        return
+
+    if cp == 0:
+        array.append(value)
+        return
+
+    if cp > 0:
+        array[i] = value
+        return
+
+    raise IndexError()
+
+
+def normalize_args(dim: Dimension, rv, kwarg_order=None) -> dict:
+    """Create an argument array from kwargs"""
+    if kwarg_order is None:
+        kwarg_order = ["loc", "scale"]
+
+    if len(dim._kwargs) == 0:
+        return dim._args[: len(kwarg_order)]
+
+    args = list(deepcopy(dim._args))
+
+    for i, kw in enumerate(kwarg_order):
+        _upsert(args, i, dim._kwargs.get(kw))
+
+    return args[: len(kwarg_order)]
+
+
 class ToConfigSpace(SpaceConverter[Hyperparameter]):
     """Convert an Orion space into a configspace"""
 
@@ -71,19 +105,19 @@ def dimension(self, dim: Dimension) -> None:
     def real(self, dim: Real) -> FloatHyperparameter:
         """Convert a real dimension into a configspace equivalent"""
         if dim.prior_name in ("reciprocal", "uniform"):
-            a, b = dim._args
+            lower, upper = dim.interval()
 
             return UniformFloatHyperparameter(
                 name=dim.name,
-                lower=a,
-                upper=b,
+                lower=lower,
+                upper=upper,
                 default_value=dim.default_value,
                 q=_qantization(dim),
                 log=dim.prior_name == "reciprocal",
             )
 
         if dim.prior_name in ("normal", "norm"):
-            a, b = dim._args
+            a, b = normalize_args(dim, dim.prior)
 
             kwargs = dict(
                 name=dim.name,
@@ -102,20 +136,21 @@ def real(self, dim: Real) -> FloatHyperparameter:
 
     def integer(self, dim: Integer) -> IntegerHyperparameter:
         """Convert a integer dimension into a configspace equivalent"""
+
         if dim.prior_name in ("int_uniform", "int_reciprocal"):
-            a, b = dim._args
+            lower, upper = dim.interval()
 
             return UniformIntegerHyperparameter(
                 name=dim.name,
-                lower=a,
-                upper=b,
+                lower=lower,
+                upper=upper,
                 default_value=dim.default_value,
                 q=_qantization(dim),
                 log=dim.prior_name == "int_reciprocal",
             )
 
         if dim.prior_name in ("int_norm", "normal"):
-            a, b = dim._args
+            a, b = normalize_args(dim, dim.prior)
 
             kwargs = dict(
                 name=dim.name,
@@ -203,12 +238,18 @@ def _from_uniform(dim: Hyperparameter) -> Integer | Real:
     else:
         kwargs["precision"] = int(-log10(dim.q)) if dim.q else 4
 
-    dist = "uniform"
-    args.append(dim.lower)
-    args.append(dim.upper)
-
     if dim.log:
         dist = "reciprocal"
+        args.append(dim.lower)
+        args.append(dim.upper)
+    else:
+        # NB: scipy uniform [loc, scale], configspace [min, max] with max = loc + scale, loc = min
+        loc = dim.lower
+        scale = dim.upper - dim.lower
+
+        dist = "uniform"
+        args.append(loc)
+        args.append(scale)
 
     return klass(dim.name, dist, *args, **kwargs)
 

tests/unittests/algo/test_configspace.py

@@ -7,18 +7,42 @@
     pytest.skip("Running without ConfigSpace", allow_module_level=True)
 
 
+def compare_spaces(s1, s2):
+    for k, original in s1.items():
+        # ConfigSpace does not have a fidelity dimension
+        # or the alpha prior
+        if k in ("f1", "a1i"):
+            continue
+
+        converted = s2[k]
+
+        # Orion space did not have default values
+        # but ConfigSpace always set them
+        if not original.default_value:
+            converted._default_value = None
+
+        assert type(original) == type(converted)
+        assert original == converted
+
+
 def test_orion_configspace():
     space = Space()
 
+    # NB: scipy uniform [loc, scale], configspace [min, max] with max = loc + scale, loc = min
+    def uniform(type, name, low, high, **kwargs):
+        return type(name, "uniform", low, high - low, **kwargs)
+
     space.register(Integer("r1i", "reciprocal", 1, 6))
-    space.register(Integer("u1i", "uniform", -3, 6))
-    space.register(Integer("u2i", "uniform", -3, 6))
-    space.register(Integer("u3i", "uniform", -3, 6, default_value=2))
+    space.register(uniform(Integer, "u1i", -3, 6))
+    space.register(uniform(Integer, "u2i", -3, 6))
+    space.register(uniform(Integer, "u4i", -4, 0, default_value=-1))
+    space.register(uniform(Integer, "u3i", -3, 6, default_value=2))
 
     space.register(Real("r1f", "reciprocal", 1, 6))
-    space.register(Real("u1f", "uniform", -3, 6))
-    space.register(Real("u2f", "uniform", -3, 6))
-    space.register(Real("name.u2f", "uniform", -3, 6))
+    space.register(uniform(Real, "u1f", -3, 6))
+    space.register(uniform(Real, "u2f", -3, 6))
+    space.register(uniform(Real, "u4f", -4, 0, default_value=-0.2))
+    space.register(uniform(Real, "name.u2f", -3, 6))
 
     space.register(Categorical("c1", ("asdfa", 2)))
     space.register(Categorical("c2", dict(a=0.2, b=0.8)))
@@ -30,24 +54,57 @@ def test_orion_configspace():
     space.register(Integer("n4", "norm", 1, 2))
 
     newspace = to_configspace(space)
-
     roundtrip = to_orionspace(newspace)
 
-    for k, original in space.items():
-        # ConfigSpace does not have a fidelity dimension
-        # or the alpha prior
-        if k in ("f1", "a1i"):
-            continue
+    compare_spaces(space, roundtrip)
 
-        converted = roundtrip[k]
 
-        # Orion space did not have default values
-        # but ConfigSpace always set them
-        if not original.default_value:
-            converted._default_value = None
+def test_orion_configspace_kwargs():
+    space = Space()
 
-        assert type(original) == type(converted)
-        assert original == converted
+    # NB: scipy uniform [loc, scale], configspace [min, max] with max = loc + scale, loc = min
+    def uniform(type, name, low, high, **kwargs):
+        return type(name, "uniform", loc=low, scale=high - low, **kwargs)
+
+    space.register(Integer("r2i", "reciprocal", a=1, b=6))
+    space.register(uniform(Integer, "u1i", -3, 6))
+    space.register(uniform(Integer, "u2i", -3, 6))
+    space.register(uniform(Integer, "u4i", -4, 0, default_value=-1))
+    space.register(uniform(Integer, "u3i", -3, 6, default_value=2))
+
+    space.register(Real("r1f", "reciprocal", a=1, b=6))
+    space.register(uniform(Real, "u1f", -3, 6))
+    space.register(uniform(Real, "u2f", -3, 6))
+    space.register(uniform(Real, "u4f", -4, 0, default_value=-0.2))
+    space.register(uniform(Real, "name.u2f", -3, 6))
+
+    space.register(Categorical("c1", categories=("asdfa", 2)))
+    space.register(Categorical("c2", categories=dict(a=0.2, b=0.8)))
+    space.register(Fidelity("f1", low=1, high=9, base=3))
+
+    space.register(Real("n1", "norm", loc=0.9, scale=0.1, precision=6))
+    space.register(Real("n2", "norm", loc=0.9, scale=0.1, precision=None))
+    space.register(Real("n3", "norm", loc=0.9, scale=0.1))
+    space.register(Integer("n4", "norm", loc=1, scale=2))
+
+    newspace = to_configspace(space)
+    r1 = to_orionspace(newspace)
+
+    newspace = to_configspace(r1)
+    r2 = to_orionspace(newspace)
+
+    # the first roundtrip conversion converted kwargs to positional arguments
+    # second roundtrip should be exactly the same
+    compare_spaces(r1, r2)
+
+    for k, original in space.items():
+        dim1 = r1.get(k)
+        dim2 = r2.get(k)
+
+        print(f"- {k:>10}", original)
+        print(" " * 12, dim1)
+        print(" " * 12, dim2)
+        print()
 
 
 def test_configspace_to_orion_unsupported():

tests/unittests/core/test_transformer.py

@@ -830,7 +830,7 @@ def test_validate(self, tdim, tdim2):
 
         with pytest.raises(ValueError) as exc:
             tdim2.validate()
-        assert "bad-default is not a valid value for this Dimension." in str(exc.value)
+        assert "bad-default is not a valid value for dimension: yolo2" in str(exc.value)
 
         tdim.original_dimension._kwargs.pop("size")
         tdim2.original_dimension._default_value = Dimension.NO_DEFAULT_VALUE