Update nevergrad parsing and output (#531)

plugins/hydra_nevergrad_sweeper/hydra_plugins/hydra_nevergrad_sweeper/config.py

@@ -6,6 +6,34 @@
 from hydra.types import ObjectConf
 
 
+@dataclass
+class ScalarConfigSpec:
+    """Representation of all the options to define
+    a scalar.
+    """
+
+    # lower bound if any
+    lower: Optional[float] = None
+
+    # upper bound if any
+    upper: Optional[float] = None
+
+    # initial value
+    # default to the middle point if completely bounded
+    init: Optional[float] = None
+
+    # step size for an update
+    # defaults to 1 if unbounded
+    # or 1/6 of the range if completely bounderd
+    step: Optional[float] = None
+
+    # cast to integer
+    integer: bool = False
+
+    # logarithmically distributed
+    log: bool = False
+
+
 @dataclass
 class OptimConf:
 

plugins/hydra_nevergrad_sweeper/hydra_plugins/hydra_nevergrad_sweeper/core.py

@@ -1,6 +1,8 @@
 # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved
+import itertools
 import logging
-from typing import Any, Dict, List, Optional, Tuple, Union
+from dataclasses import dataclass
+from typing import Any, Dict, List, Optional, Tuple
 
 from hydra.core.config_loader import ConfigLoader
 from hydra.core.config_search_path import ConfigSearchPath
@@ -11,7 +13,7 @@
 from hydra.types import TaskFunction
 from omegaconf import DictConfig, ListConfig, OmegaConf
 
-from .config import OptimConf
+from .config import OptimConf, ScalarConfigSpec
 
 # pylint: disable=logging-fstring-interpolation,no-self-used
 
@@ -25,41 +27,106 @@ def manipulate_search_path(self, search_path: ConfigSearchPath) -> None:
         )
 
 
-def convert_to_deduced_type(string: str) -> Union[int, float, str]:
-    """Converts a string into a float or an int if it makes sense,
-    or returns the string.
+@dataclass
+class CommandlineSpec:
+    """Structured commandline specification
+    for sweepers handling categorical variables and bounded variables
 
-    Examples
-    --------
-    read_value("1")
-    >>> 1
-
-    read_value("1.0")
-    >>> 1.0
+    Attributes
+    ----------
+    bounds: Optional[Tuple[float, float]]
+        if present, this defines a bounded scalar between bounds[0]
+        and bounds[1]
+    options: Optional[List[Any]]
+        if present, this defines the options/choices of a categorical
+        variable
+    cast: str
+        the name of the variable type to cast it to ("int", "str"
+        or "float")
+    log: bool
+        for bounded scalars, whether it is log-distributed
 
-    read_value("blublu")
-    >>> "blublu"
+    Note
+    ----
+    Exactly one of bounds or options must be provided
     """
-    output: Union[float, int, str] = string
-    if not isinstance(string, str):
-        return string
-    try:
-        output = float(string)
-    except ValueError:
-        pass
-    else:
-        if output.is_integer() and "." not in string:
-            output = int(output)
-    return output
-
-
-def make_parameter_from_commandline(string: str) -> Any:
-    """Returns a Nevergrad parameter from a definition string.
+
+    bounds: Optional[Tuple[float, float]] = None
+    options: Optional[List[str]] = None
+    cast: str = "float"
+    log: bool = False
+
+    def __post_init__(self) -> None:
+        if not (self.bounds is None) ^ (self.options is None):
+            raise ValueError("Exactly one of bounds or options must be specified")
+        if self.bounds is not None:
+            if self.cast == "str":
+                raise ValueError(
+                    "Inconsistent specifications 'str' for bounded values."
+                )
+            if self.bounds[0] > self.bounds[1]:
+                raise ValueError(f"Bounds must be ordered, but got {self.bounds}")
+        if self.options is not None and self.log:
+            raise ValueError("Inconsistent 'log' specification for choice parameter")
+
+    @classmethod
+    def parse(cls, string: str) -> "CommandlineSpec":
+        """Parses a commandline argument string
+
+        Parameter
+        ---------
+        string: str
+            This can be:
+             - comma-separated values: for a choice parameter
+               Eg.: "a,b,c"
+             - colon-separated values for ranges of scalars.
+               Eg.: "0:10"
+            Colon-separeted can be appended to:
+             - cast to int/str/float (always defaults to float):
+               Eg: "float:0,4,10", "int:0:10"
+             - set log distribution for scalars
+               Eg: "int:log:4:1024"
+        """
+        available_modifiers = {"log", "float", "int", "str"}
+        colon_split = string.split(":")
+        modifiers = set(
+            itertools.takewhile(available_modifiers.__contains__, colon_split)
+        )
+        remain = colon_split[len(modifiers) :]
+        casts = list(modifiers - {"log"})
+        if len(remain) not in {1, 2}:
+            raise ValueError(
+                "Can't interpret non-speficiations: {}.\nthis needs to be "
+                "either colon or coma-separated values".format(":".join(remain))
+            )
+        if len(casts) > 1:
+            raise ValueError(f"Inconsistent specifications: {casts}")
+        if len(remain) == 1:  # choice argument
+            cast = casts[0] if casts else "str"
+            options = remain[0].split(",")
+            if len(options) < 2:
+                raise ValueError("At least 2 options are required")
+            if not casts:
+                try:  # default to float if possible and no spec provided
+                    _ = [float(x) for x in options]
+                    cast = "float"
+                except ValueError:
+                    pass
+            return cls(options=options, cast=cast)
+        # bounded argument
+        bounds: Tuple[float, float] = tuple(float(x) for x in remain)  # type: ignore
+        cast = casts[0] if casts else "float"
+        return cls(bounds=bounds, cast=cast, log="log" in modifiers)
+
+
+# pylint: disable=too-many-branches
+def make_nevergrad_parameter(description: Any) -> Any:
+    """Returns a Nevergrad parameter from a definition string or object.
 
     Parameters
     ----------
-    string: str
-         a definition string. This can be:
+    description: Any
+       * a commandline definition string. This can be:
          - comma-separated values: for a choice parameter
            Eg.: "a,b,c"
            Note: sequences of increasing scalars provide a specific parametrization
@@ -71,70 +138,74 @@ def make_parameter_from_commandline(string: str) -> Any:
            runtime. This provides full nevergrad flexibility at the cost of robustness.
            Eg.:"Log(a_min=0.001, a_max=0.1)"
          - anything else will be treated as a constant string
+       * a config definition dict for scalar parameters, with potential fields
+         init, lower, upper, step, log, integer
+       * a list for option parameters defined in config file
 
     Returns
     -------
     Parameter or str
         A Parameter if the string fitted one of the definitions, else the input string
     """
-    string = string.strip()
-    if "," in string:
-        return make_parameter_from_config(string.split(","))
-    if ":" in string:
-        *specs, b1, b2 = string.split(":")
-        a, b = [convert_to_deduced_type(x) for x in (b1, b2)]
-        assert isinstance(a, (int, float)), "Bounds must be scalars"
-        assert isinstance(b, (int, float)), "Bounds must be scalars"
-        description = {"lower": a, "upper": b, "log": "log" in specs}
-        if "int" in specs:
-            description["integer"] = True
-            if not all(isinstance(x, int) for x in (a, b)):
-                raise TypeError(
-                    f"Only integers should be provided for integer ranges (got {string})"
-                )
-            if b - a <= 6:
-                raise ValueError(
-                    "For integers with 6 or fewer values, use a choice instead"
-                )
-        return make_parameter_from_config(description)
-    return make_parameter_from_config(string)
-
-
-def make_parameter_from_config(description: Any) -> Any:
     # lazy initialization to avoid overhead when loading hydra
     import nevergrad as ng
 
-    # choice
+    # revert config parsing
 
     if isinstance(description, (ListConfig, list)):
-        choices = [convert_to_deduced_type(x) for x in description]
-        ordered = all(isinstance(c, (int, float)) for c in choices)
-        ordered &= all(
-            c0 <= c1 for c0, c1 in zip(choices[:-1], choices[1:])  # type: ignore
+        description = ",".join(description)
+    if isinstance(description, str):
+        # hacky nevergrad parameter
+        if description.startswith(tuple(dir(ng.p))):
+            param: ng.p.Parameter = eval(  # pylint: disable=eval-used
+                "ng.p." + description
+            )
+            assert isinstance(param, ng.p.Parameter)
+            return param
+        # cast to spec if possible
+        try:
+            description = CommandlineSpec.parse(description)
+        except ValueError:
+            pass
+    # convert scalar commandline specs to dict
+    if isinstance(description, CommandlineSpec) and description.bounds is not None:
+        description = ScalarConfigSpec(
+            lower=description.bounds[0],
+            upper=description.bounds[1],
+            log=description.log,
+            integer=description.cast == "int",
         )
-        return ng.p.TransitionChoice(choices) if ordered else ng.p.Choice(choices)
-    # custom
-    if isinstance(description, str) and description.startswith(tuple(dir(ng.p))):
-        param: ng.p.Parameter = eval("ng.p." + description)  # pylint: disable=eval-used
-        assert isinstance(param, ng.p.Parameter)
-        return param
-    # scalar
+    # convert scalar config specs to dict
+    # convert dict to Scalar parameter instance
     if isinstance(description, (dict, DictConfig)):
+        description = ScalarConfigSpec(**description)
+    if isinstance(description, ScalarConfigSpec):
         init = ["init", "lower", "upper"]
-        options = init + ["log", "step", "integer"]
-        assert all(x in options for x in description)
-        init_params = {x: y for x, y in description.items() if x in init}
-        if not description.get("log", False):
+        init_params = {x: getattr(description, x) for x in init}
+        if not description.log:
             scalar = ng.p.Scalar(**init_params)
-            if "step" in description:
-                scalar.set_mutation(sigma=description["step"])
+            if description.step is not None:
+                scalar.set_mutation(sigma=description.step)
         else:
-            if "step" in description:
-                init_params["exponent"] = description["step"]
+            if description.step is not None:
+                init_params["exponent"] = description.step
             scalar = ng.p.Log(**init_params)
-        if description.get("integer", False):
+        if description.integer:
             scalar.set_integer_casting()
+            a, b = scalar.bounds
+            if a is not None and b is not None and b - a <= 6:
+                raise ValueError(
+                    "For integers with 6 or fewer values, use a choice instead"
+                )
         return scalar
+    # choices
+    if isinstance(description, CommandlineSpec):
+        assert description.options is not None
+        caster = {"int": int, "str": str, "float": float}[description.cast]
+        choices = [caster(x) for x in description.options]
+        ordered = all(isinstance(c, (int, float)) for c in choices)
+        ordered &= all(c0 <= c1 for c0, c1 in zip(choices[:-1], choices[1:]))
+        return ng.p.TransitionChoice(choices) if ordered else ng.p.Choice(choices)
     # constant
     if isinstance(description, (str, int, float)):
         return description
@@ -166,7 +237,7 @@ def __init__(
         if parametrization is not None:
             assert isinstance(parametrization, DictConfig)
             self.parametrization = {
-                x: make_parameter_from_config(y) for x, y in parametrization.items()
+                x: make_nevergrad_parameter(y) for x, y in parametrization.items()
             }
         self.job_idx: Optional[int] = None
 
@@ -195,7 +266,7 @@ def sweep(self, arguments: List[str]) -> None:
         params = dict(self.parametrization)
         for s in arguments:
             key, value = s.split("=", 1)
-            params[key] = make_parameter_from_commandline(value)
+            params[key] = make_nevergrad_parameter(value)
         parametrization = ng.p.Dict(**params)
         parametrization.descriptors.deterministic_function = not self.opt_config.noisy
         parametrization.random_state.seed(self.opt_config.seed)
@@ -233,8 +304,8 @@ def sweep(self, arguments: List[str]) -> None:
         recom = optimizer.provide_recommendation()
         results_to_serialize = {
             "name": "nevergrad",
-            "best_parameters": best[1].value,
-            "best_achieved_result": direction * best[0],
+            "best_evaluated_params": best[1].value,
+            "best_evaluated_result": direction * best[0],
         }
         OmegaConf.save(
             OmegaConf.create(results_to_serialize),

plugins/hydra_nevergrad_sweeper/tests/test_commandline_spec.py

@@ -0,0 +1,33 @@
+# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved
+from dataclasses import asdict
+
+import pytest  # type: ignore
+
+from hydra_plugins.hydra_nevergrad_sweeper.core import CommandlineSpec
+
+
+@pytest.mark.parametrize(  # type: ignore
+    "string,expected",
+    [
+        ("blu,blublu", CommandlineSpec(options=["blu", "blublu"], cast="str")),
+        ("0,1,2", CommandlineSpec(options=["0", "1", "2"], cast="float")),
+        ("str:0,1,2", CommandlineSpec(options=["0", "1", "2"], cast="str")),
+        (
+            "0.0,12.0,2.0",
+            CommandlineSpec(options=["0.0", "12.0", "2.0"], cast="float"),
+        ),
+        ("int:1:12", CommandlineSpec(bounds=(1.0, 12.0), cast="int")),
+        ("1:12", CommandlineSpec(bounds=(1.0, 12.0), cast="float")),
+        ("log:0.01:1.0", CommandlineSpec(bounds=(0.01, 1.0), cast="float", log=True),),
+        (
+            "int:log:1:1200",
+            CommandlineSpec(bounds=(1.0, 1200.0), cast="int", log=True),
+        ),
+    ],
+)
+def test_commandline_spec_parse(string: str, expected: CommandlineSpec) -> None:
+    param = CommandlineSpec.parse(string)
+    for attr, value in asdict(expected).items():
+        assert getattr(param, attr) == value, f"Wrong value for {attr}"
+    # the following equality fails for unknown reasons (only if running all tests)
+    # assert param == expected