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

Fix issue with subsolvers whose hyperparameters share names but not types #221

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

nhuet
Copy link
Contributor

@nhuet nhuet commented Jun 6, 2024

Optuna suggestion could raise error when a subbrick could take 2 different classes that have both an hyperparameter with same name but of a different kind. (For instance one being an IntegerHyperparameter, the other being a CategoricalHyperparameter).

To avoid this issue, we update the hyperparameter prefix when seen as a parameter of an optuna trial to include the actual class name of the chosen subbrick.

For example,

class MySolver(SolverDO):
    hyperparameters = [
        SubBrickHyperparameter("subsolver", choices=[LPSolver, CPSolver]),
        SubBrickKwargsHyperparameter(
            "kwargs_subsolver", subbrick_hyperparameter="subsolver"
        ),
    ]

    ...

class LPSolver(SolverDO):
    hyperparameters = [
        CategoricalHyperparameter("hp", choices=[True, False]),
    ]
    ...

class CPSolver(SolverDO):
    hyperparameters = [
        IntegerHyperparameter("hp", low=0, high=10),
        FloatHyperparameter("coeff", low=0., high=1.),
    ]
    ...

Here the trial params will be

  • first trial: (subsolver, subsolver.LPSolver.hp)
  • second trial: (subsolver, subsolver.CPSolver.hp, subsolver.CPSolver.coeff)

We modify DummySolver2 in test_hyperparameters.py so that the issue raised in the tests before the fix and the tests are ok with it.

NB: The issue could still arise if a subbrick could be of two different classes with same name (but from different modules) that also share same hyperparameter name of different type. This is higly unlikely to happen and could not justify having the whole path including the module to be inserted in the prefix. This would lead to a very unreadable parameter name in optuna trial.

Optuna suggestion could raise error when a subbrick could take 2
different classes that have both an hyperparameter with same name but of a
different kind. (For instance one being an IntegerHyperparameter, the
other being a CategoricalHyperparameter).

To avoid this issue, we update the hyperparameter prefix when seen as a
parameter of an optuna trial to include the actual class name of the
chosen subbrick.

For example,

class MySolver(SolverDO):
    hyperparameters = [
        SubBrickHyperparameter("subsolver", choices=[LPSolver, CPSolver]),
        SubBrickKwargsHyperparameter(
            "kwargs_subsolver", subbrick_hyperparameter="subsolver"
        ),
    ]

    ...

class LPSolver(SolverDO):
    hyperparameters = [
        CategoricalHyperparameter("hp", choices=[True, False]),
    ]
    ...

class CPSolver(SolverDO):
    hyperparameters = [
        IntegerHyperparameter("hp", low=0, high=10),
        FloatHyperparameter("coeff", low=0., high=1.),
    ]
    ...

Here the trial params will be

- first trial: (subsolver, subsolver.LPSolver.hp)
- second trial: (subsolver, subsolver.CPSolver.hp, subsolver.CPSolver.coeff)

We modify DummySolver2 in test_hyperparameters so that the issue raised
in the tests before the fix and the tests are ok with it.

NB: The issue could still arise if a subbrick could be of two different
classes with same name (but from different modules) that also share same
hyperparameter name of different type. This is higly unlikely to happen
and could not justify having the whole path including the module to be
inserted in the prefix. This would lead to a very unreadable parameter
name in optuna trial.
@nhuet nhuet force-pushed the subbrickkwargs-changing-space branch from 7fbc2b4 to ae4bdba Compare June 6, 2024 09:16
Copy link
Collaborator

@g-poveda g-poveda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thx

@g-poveda g-poveda merged commit 9187eb9 into airbus:master Jun 6, 2024
14 checks passed
@nhuet nhuet deleted the subbrickkwargs-changing-space branch June 6, 2024 11:55
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 this pull request may close these issues.

2 participants