Simplify Parameter.copy() (#1048)

.circleci/config.yml

@@ -172,7 +172,7 @@ jobs:
           command: |
             . venv/bin/activate
             pylint --version
-            pylint nevergrad --disable=all --enable=unused-import,unused-argument,unused-variable,undefined-loop-variable,redefined-builtin,used-before-assignment,super-init-not-called,useless-super-delegation,dangerous-default-value
+            pylint nevergrad --disable=all --enable=unused-import,unused-argument,unused-variable,undefined-loop-variable,redefined-builtin,used-before-assignment,super-init-not-called,useless-super-delegation,dangerous-default-value,unnecessary-pass
 
       - run:
           name: "Run black"

CHANGELOG.md

@@ -12,6 +12,7 @@
   [#1043](https://github.com/facebookresearch/nevergrad/pull/1043)
   [#1044](https://github.com/facebookresearch/nevergrad/pull/1044)
   and more to come), please open an issue if you encounter any problem. The midterm aim is to allow for simpler constraint management.
+- `copy()` method of a `Parameter` does not change the parameters's random state anymore (it used to reset it to `None` [#1048](https://github.com/facebookresearch/nevergrad/pull/1048)
 
 ## 0.4.3 (2021-01-28)
 

nevergrad/benchmark/xpbase.py

@@ -12,7 +12,8 @@
 import typing as tp
 import numpy as np
 from nevergrad.parametrization import parameter as p
-from ..common import decorators
+from nevergrad.common import decorators
+from nevergrad.common import errors
 from ..functions.rl.agents import torch  # import includes pytorch fix
 from ..functions import base as fbase
 from ..optimization import base as obase
@@ -193,7 +194,7 @@ def run(self) -> tp.Dict[str, tp.Any]:
         """
         try:
             self._run_with_error()
-        except (fbase.ExperimentFunctionCopyError, fbase.UnsupportedExperiment) as ex:
+        except (errors.ExperimentFunctionCopyError, errors.UnsupportedExperiment) as ex:
             raise ex
         except Exception as e:  # pylint: disable=broad-except
             # print the case and the traceback
@@ -255,7 +256,7 @@ def _run_with_error(self, callbacks: tp.Optional[tp.Dict[str, obase._OptimCallBa
         executor = self.optimsettings.executor
         with warnings.catch_warnings():
             warnings.filterwarnings(
-                "ignore", category=obase.errors.InefficientSettingsWarning
+                "ignore", category=errors.InefficientSettingsWarning
             )  # benchmark do not need to be efficient
             try:
                 # call the actual Optimizer.minimize method because overloaded versions could alter the worklflow

nevergrad/functions/base.py

@@ -6,25 +6,24 @@
 import inspect
 from pathlib import Path
 import numbers
-import unittest
 import numpy as np
 import nevergrad.common.typing as tp
+from nevergrad.common import errors
+from nevergrad.common.errors import (  # pylint: disable=unused-import
+    UnsupportedExperiment as UnsupportedExperiment,
+)
 from nevergrad.parametrization import parameter as p
 from nevergrad.optimization import multiobjective as mobj
 
 EF = tp.TypeVar("EF", bound="ExperimentFunction")
 ME = tp.TypeVar("ME", bound="MultiExperiment")
 
 
-class ExperimentFunctionCopyError(NotImplementedError):
-    """Raised when the experiment function fails to copy itself (for benchmarks)"""
-
-
-class UnsupportedExperiment(RuntimeError, unittest.SkipTest):
-    """Raised if the experiment is not compatible with the current settings:
-    Eg: missing data, missing import, unsupported OS etc
-    This automatically skips tests.
-    """
+def _reset_copy(obj: p.Parameter) -> p.Parameter:
+    """Copy a parameter and resets its random state to obtain variability"""
+    out = obj.copy()
+    out.random_state = None
+    return out
 
 
 # pylint: disable=too-many-instance-attributes
@@ -159,7 +158,7 @@ def _internal_copy(self: EF) -> EF:
         """
         # auto_init is automatically filled by __new__, aka when creating the instance
         output: EF = self.__class__(
-            **{x: y.copy() if isinstance(y, p.Parameter) else y for x, y in self._auto_init.items()}
+            **{x: _reset_copy(y) if isinstance(y, p.Parameter) else y for x, y in self._auto_init.items()}
         )
         return output
 
@@ -178,10 +177,10 @@ def copy(self: EF) -> EF:
         # parametrization may have been overriden, so let's always update it
         # Caution: only if names differ!
         if output.parametrization.name != self.parametrization.name:
-            output.parametrization = self.parametrization.copy()
+            output.parametrization = _reset_copy(self.parametrization)
         # then if there are still differences, something went wrong
         if not output.equivalent_to(self):
-            raise ExperimentFunctionCopyError(
+            raise errors.ExperimentFunctionCopyError(
                 f"Copy of\n{self}\nwith descriptors:\n{self._descriptors}\nreturned non-equivalent\n"
                 f"{output}\nwith descriptors\n{output._descriptors}.\n\n"
                 "This means that the auto-copy behavior of ExperimentFunction does not work.\n"

nevergrad/functions/images/imagelosses.py

@@ -30,7 +30,6 @@ def __init__(self, reference: tp.Optional[np.ndarray] = None) -> None:
             assert self.reference.max() <= 256.0, f"Image max = {self.reference.max()}"
             assert self.reference.max() > 3.0  # Not totally sure but entirely black images are not very cool.
             self.domain_shape = self.reference.shape
-        pass
 
     def __call__(self, img: np.ndarray) -> float:
         raise NotImplementedError(f"__call__ undefined in class {type(self)}")

nevergrad/optimization/es.py

@@ -66,10 +66,8 @@ def _internal_tell_candidate(self, candidate: p.Parameter, loss: float) -> None:
             if loss < parent_value:
                 self._population[uid] = candidate
         else:
-            if (
-                candidate.parents_uids[0] not in self._population
-                and len(self._population) < self._config.popsize
-            ):
+            no_parent = next(iter(candidate.parents_uids), "#no_parent#") not in self._population
+            if no_parent and len(self._population) < self._config.popsize:
                 self._population[candidate.uid] = candidate
                 self._uid_queue.tell(candidate.uid)
             else:

nevergrad/optimization/recorded_recommendations.csv

@@ -125,7 +125,6 @@ NGOpt10,1.012515477,-0.9138805701,-1.029555946,1.2098418178,,,,,,,,,,,,
 NGOpt4,1.012515477,-0.9138805701,-1.029555946,1.2098418178,,,,,,,,,,,,
 NGOpt8,1.012515477,-0.9138805701,-1.029555946,1.2098418178,,,,,,,,,,,,
 NGOptBase,0.0,-0.3451057176,-0.1327329683,1.9291307781,,,,,,,,,,,,
-NonNSGAIIES,1.1400386808,0.3380024444,0.4755144618,2.6390460807,0.6911075733,1.111235567,-0.2576843178,-1.1959512855,,,,,,,,
 NaiveAnisoEMNA,1.012515477,-0.9138691467,-1.0295302074,1.2097964496,,,,,,,,,,,,
 NaiveAnisoEMNATBPSA,0.002380178,-0.0558141,-0.3746306258,1.3332040355,,,,,,,,,,,,
 NaiveIsoEMNA,1.012515477,-0.9138691467,-1.0295302074,1.2097964496,,,,,,,,,,,,
@@ -136,6 +135,7 @@ NoisyBandit,1.012515477,-0.9138691467,-1.0295302074,1.2097964496,,,,,,,,,,,,
 NoisyDE,0.7325595717,-0.3250848292,-0.4968122173,1.9884218193,1.8577990761,1.7725922124,-0.966685952,-1.5886443264,,,,,,,,
 NoisyDiscreteOnePlusOne,0.7531428339,0.0,0.0,0.0,,,,,,,,,,,,
 NoisyOnePlusOne,0.0,0.0,0.0,0.0,,,,,,,,,,,,
+NonNSGAIIES,1.1400386808,0.3380024444,0.4755144618,2.6390460807,0.6911075733,1.111235567,-0.2576843178,-1.1959512855,,,,,,,,
 ORandomSearch,-0.4729858315,0.6814258794,-0.2424394967,1.700735634,,,,,,,,,,,,
 OScrHammersleySearch,-0.9674215661,0.0,0.4307272993,0.8416212336,,,,,,,,,,,,
 OnePlusOne,1.0082049151,-0.9099785499,-1.025147209,1.2046460074,,,,,,,,,,,,

nevergrad/parametrization/core.py

@@ -11,7 +11,7 @@
 from nevergrad.common import errors
 from . import utils
 
-# pylint: disable=no-value-for-parameter
+# pylint: disable=no-value-for-parameter,pointless-statement
 
 
 P = tp.TypeVar("P", bound="Parameter")
@@ -148,9 +148,10 @@ def sample(self: P) -> P:
         This function should be used in optimizers when creating an initial population,
         and parameter.heritage["lineage"] is reset to parameter.uid instead of its parent's
         """
-        child = self.spawn_child()
+        self.random_state  # make sure to populate it before copy
+        child = self.copy()
+        child._set_parenthood(None)
         child.mutate()
-        child.heritage["lineage"] = child.uid
         return child
 
     def recombine(self: P, *others: P) -> None:
@@ -259,7 +260,7 @@ def get_value_hash(self) -> tp.Hashable:
         if isinstance(val, (str, bytes, float, int)):
             return val
         elif isinstance(val, np.ndarray):
-            return val.tobytes()  # type: ignore
+            return val.tobytes()
         else:
             raise errors.UnsupportedParameterOperationError(
                 f"Value hash is not supported for object {self.name}"
@@ -381,16 +382,24 @@ def spawn_child(self: P, new_value: tp.Optional[tp.Any] = None) -> P:
             a new instance of the same class, with same content/internal-model parameters/...
             Optionally, a new value will be set after creation
         """
-        # make sure to initialize the random state  before spawning children
-        self.random_state  # pylint: disable=pointless-statement
+        self.random_state  # make sure to initialize the random state  before spawning children
+        child = self.copy()
+        child._set_parenthood(self)
+        if new_value is not None:
+            child.value = new_value
+        return child
+
+    def copy(self: P) -> P:
+        """Creates a full copy of the parameter.
+        Use spawn_child instead to make sure to add the parenthood information.
+        """
         child = copy.copy(self)
         child.uid = uuid.uuid4().hex
         child._frozen = False
-        child._generation += 1
-        child.parents_uids = [self.uid]
-        child.heritage = dict(self.heritage)
         child._subobjects = self._subobjects.new(child)
         child._meta = {}
+        child.parents_uids = list(self.parents_uids)
+        child.heritage = dict(self.heritage)
         child.loss = None
         child._losses = None
         child._constraint_checkers = list(self._constraint_checkers)
@@ -400,11 +409,20 @@ def spawn_child(self: P, new_value: tp.Optional[tp.Any] = None) -> P:
             container = dict(container) if isinstance(container, dict) else list(container)
             setattr(child, attribute, container)
         for key, val in self._subobjects.items():
-            container[key] = val.spawn_child()
-        if new_value is not None:
-            child.value = new_value
+            container[key] = val.copy()
         return child
 
+    def _set_parenthood(self, parent: tp.Optional["Parameter"]) -> None:
+        """Sets the parenthood information to Parameter and subparameters."""
+        if parent is None:
+            self._generation = 0
+            self.heritage = dict(lineage=self.uid)
+            self.parents_uids = []
+        else:
+            self._generation = parent.generation + 1
+            self.parents_uids = [parent.uid]
+        self._subobjects.apply("_set_parenthood", parent)
+
     def freeze(self) -> None:
         """Prevents the parameter from changing value again (through value, mutate etc...)"""
         self._frozen = True
@@ -420,14 +438,6 @@ def _check_frozen(self) -> None:
             )
         self._subobjects.apply("_check_frozen")
 
-    def copy(self: P) -> P:  # TODO test (see former instrumentation_copy test)
-        """Create a child, but remove the random state
-        This is used to run multiple experiments
-        """
-        child = self.spawn_child()
-        child.random_state = None
-        return child
-
     def _compute_descriptors(self) -> utils.Descriptors:
         return utils.Descriptors()
 

nevergrad/parametrization/data.py

@@ -392,11 +392,9 @@ def recombine(self: D, *others: D) -> None:
         else:
             raise ValueError(f'Unknown recombination "{recomb}"')
 
-    def spawn_child(self: D, new_value: tp.Optional[tp.Any] = None) -> D:
-        child = super().spawn_child()  # dont forward the value
+    def copy(self: D) -> D:
+        child = super().copy()
         child._value = np.array(self._value, copy=True)
-        if new_value is not None:
-            child.value = new_value
         return child
 
 

nevergrad/parametrization/test_parameter.py

@@ -403,3 +403,12 @@ def test_array_sampling(method: str, exponent: tp.Optional[float], sigma: float)
         assert np.any(np.abs(val) > 10)
         assert np.all(val <= mbound)
         assert np.all(val >= 1)
+
+
+def test_parenthood() -> None:
+    param = par.Instrumentation(par.Scalar(init=1.0, mutable_sigma=True).set_mutation(exponent=2.0, sigma=5))
+    sigma_uid = param[0][0].sigma.uid  # type: ignore
+    param_samp = param.sample()
+    param_spawn = param.spawn_child()
+    assert param_samp[0][0].sigma.parents_uids == []  # type: ignore
+    assert param_spawn[0][0].sigma.parents_uids == [sigma_uid]  # type: ignore