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

Proposition: Rework how configuration is saved through BaseAlgorithm.__init__ #860

Closed
lebrice opened this issue Mar 28, 2022 · 1 comment · Fixed by #867
Closed

Proposition: Rework how configuration is saved through BaseAlgorithm.__init__ #860

lebrice opened this issue Mar 28, 2022 · 1 comment · Fixed by #867
Milestone

Comments

@lebrice
Copy link
Collaborator

lebrice commented Mar 28, 2022

The current mechanism for storing the configuration of a custom algorithm is to pass all the kwargs to super().__init__(**kwargs), like so:

def __init__(
        self,
        space: Space,
        n_values: int | dict[str, int] = 100,
        seed: int | Sequence[int] | None = None,
    ):
        super().__init__(space, n_values=n_values, seed=seed)
        self.n = 0
        ...
        something = foo(self.n_values)   # type checker complains that `self.n_values` can't be found

The __init__ of BaseAlgorithm then saves the names of the arguments passed in an attribute, and then sets the values on self:

def __init__(self, space, **kwargs):
        log.debug(
            "Creating Algorithm object of %s type with parameters:\n%s",
            type(self).__name__,
            kwargs,
        )
        self._space = space
        self._param_names = list(kwargs.keys())
        # Instantiate tunable parameters of an algorithm
        for varname, param in kwargs.items():
            setattr(self, varname, param)

        # TODO: move this inside an initialization function.
        if hasattr(self, "seed"):
            self.seed_rng(self.seed)

        self.registry = Registry()

I see one main disadvantage of doing things this way:

  • When using attributes set in this way, type checkers complain that the attribute is not defined (since it doesn't have a self.n_values = n_values, for example.

I'd also argue that it seems a bit too much responsibility to give to BaseAlgorithm.__init__ to save the names, set the values, and initialize the 'base' attributes like self.registry, etc.

Suggestion:

I suggest that we still allow this mechanism, but that we additionally allow saving the attributes directly on self instead and discover _param_names programmatically, like so:

self._arg_names = list(kwargs or inspect.signature(type(self)).parameters)

_arg_names will contain the names of all the constructor arguments.

@lebrice
Copy link
Collaborator Author

lebrice commented Mar 28, 2022

Oh and while we're at it, I suggest we add a self.max_trials: Optional[int] = None attribute in BaseAlgorithm. Lots of tests forcefully set this attribute on the algos, even though it isn't defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants