Refactor NSGA-II crossover operations into classes #3221

xadrianzetx · 2022-01-09T10:34:58Z

Motivation

This PR aims to create common API for all NSGA-II crossover operations and refactor functions introduced in #2903 into classes following this API. This decouples crossover implementations from sampler implementation, making them modular, easier to maintain/test/extend. Also, some numpy operations are vectorized (straightforward ones at least). This is a part of #3133.

Description of the changes

Introduce common crossover API
Refactor crossover functions into classes following this API
Make NSGAIISampler support new crossover format
Docs
Tests

This class forms common API for all existing and future NSGA-II crossover operations.

xadrianzetx · 2022-01-09T10:35:42Z

This is still early work-in-progress, but general design should be clearly visible by now, so any comments on it would be greatly appreciated.

codecov-commenter · 2022-01-15T19:18:02Z

Codecov Report

Merging #3221 (1b958cf) into master (6584ff5) will decrease coverage by 0.02%.
The diff coverage is 96.11%.

@@            Coverage Diff             @@
##           master    #3221      +/-   ##
==========================================
- Coverage   91.72%   91.70%   -0.03%     
==========================================
  Files         146      154       +8     
  Lines       12065    12132      +67     
==========================================
+ Hits        11067    11126      +59     
- Misses        998     1006       +8

Impacted Files	Coverage Δ
optuna/samplers/_base.py	`82.14% <ø> (ø)`
optuna/samplers/nsgaii/_crossovers/_base.py	`84.61% <84.61%> (ø)`
optuna/samplers/nsgaii/_crossover.py	`91.78% <91.78%> (ø)`
optuna/samplers/nsgaii/_crossovers/_vsbx.py	`96.96% <96.96%> (ø)`
optuna/samplers/nsgaii/_crossovers/_sbx.py	`97.67% <97.67%> (ø)`
optuna/samplers/nsgaii/_crossovers/_undx.py	`98.07% <98.07%> (ø)`
optuna/samplers/__init__.py	`100.00% <100.00%> (ø)`
optuna/samplers/nsgaii/__init__.py	`100.00% <100.00%> (ø)`
optuna/samplers/nsgaii/_crossovers/_blxalpha.py	`100.00% <100.00%> (ø)`
optuna/samplers/nsgaii/_crossovers/_spx.py	`100.00% <100.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6584ff5...1b958cf. Read the comment docs.

Sharp inequality is needed to follow spec

toshihikoyanase · 2022-01-21T01:49:15Z

@HideakiImamura @sile Could you review this PR, please? If you don't have enough time, please remove the assignment.

sile · 2022-01-21T04:04:51Z

Yes. I'll review this PR this or next weekend.

HideakiImamura

Thank you for the update. I checked all of your changes. I have several minor comments. PTAL.

optuna/samplers/nsga2/_crossovers/_base.py

tests/samplers_tests/test_nsga2.py

xadrianzetx · 2022-01-28T17:37:48Z

Seems like workflows are stuck. Could you guys re-trigger them?

docs/source/reference/samplers/nsga2.rst

sile · 2022-01-28T23:48:29Z

optuna/samplers/nsga2/_crossovers/_base.py

+        parents_params: np.ndarray,
+        rng: np.random.RandomState,
+        study: Study,
+        search_space: Dict[str, BaseDistribution],


It seems, in numerical distributions case, parents_params contains parameters transformed by _SearchSpaceTransform.transform method. In such a case, the upper and lower bounds of each transformed parameter are not guaranteed to be the same as the original distribution (especially if suggest_float(..., log=True) or suggest_float(..., step=...) is used to sample the parameter).

So passing the (non-transformed) distributions here could cause an unexpected calculation if a crossover implementation uses the low or high fields of the distributions (e.g., _sbx.py#72). I think that we need to use an instance of _SearchSpaceTransform (or __SearchSpaceTransform.bounds) in the crossover calculation instead of the Dict[str, BaseDistribution] instance (for numerical parameters).

A difficult point is that, in categorical distributions case, we should keep using Dict[str, BaseDistrivution] because we don't apply the transformation on categorical parameters.

Just an idea, but we may be able to define a function converting __SearchSpaceTransform.bounds to List[BaseDistribution] and apply the function to search_space before passing to the crossover method.

Since _SearchSpaceTransform already has an information about search space and bounds, maybe it should be able to do this conversion? I could implement this method in separate PR and merge it back here.

Or not, since it might not be possible to instantiate a distribution with transformed bounds e.g. FloatDistribution(log(1), log(10), log=True). I think we might be forced to pass instance of _SearchSpaceTransform after all. Maybe we could make OHE optional (_SearchSpaceTransform with ohe=False would be just identity function for categorical params)? We could then use it regardless of distribution, and pass to crossover instead of search space dict.

@hvy As the implementor of _SearchSpaceTransform, do you have any preference for how to deal with this issue?

along with making _SearchSpaceTransform public

I think I could work on this right now in separate PR. This would be really usefull as now public SearchSpaceTransform would have essentially the functionality of bounds wrapper I was talking about previously. I think in the process of making it public we could think about some convenience methods such as __len__ to return search space length. What do you think about discussing and implementing it in separate PR? It would be a drop in replacement for search_space dict.

we could make it more Optuna-like by passing arguments that more resemble those of _try_crossover

Passing some object with params instead of an array in parents_params? +1 to that in future revisions, as it would probably come in handy if other crossovers were to support OHE.

What do you think about discussing and implementing it in separate PR?

Sounds good. I think it'll be important to hear what others are thinking about this, considering the fact that this'll likely be an API targeting only a small portion of the users. __len__ by the way sounds interesting. I'd personally prefer named methods, as it's not entirely clear from __len__ whether it will return the length after or prior to the transformation, but this is not a strong opinion.

Given the discussion so far, how do you suggest we proceed with this PR. Should we aim to merge it as is, revisit it at a later time? If so, should we perhaps make APIs in this PR experimental to reserve ourselves for changes? Alternatively, we'll keep this PR open until everything discussed so far is fixed.

+1 to make _SearchSpaceTransform public. I think it would be beneficial for users to make it public, since it could be used by users who want to implement a new sampler, for example.

On the other hand, I don't think merging this PR will have to wait for that. Frankly speaking, @xadrianzetx's contribution to Optuna V3 is very big and we are relying on your contribution a lot. Making _SearchSpaceTransform public is out of scope for Optuna V3, but perhaps PR is expected to be somewhat bigger, including the interface. Therefore, in this PR, why not perform an in-line uniform crossover on categorical parameters and pass _SearchSpaceTransform.bounds to try_crossover? I suggest that making _SearchSpaceTransform public is a separate issue to be made into future work.

Fully agree with @HideakiImamura.

Uniform crossover for categorical params will be inlined, and BaseCrossover implementations will be reserved for transformed numerical params (for now).

We will re-design search_space arg to accept numpy array of _SearchSpaceTransform.bounds instead of distribution dict to solve issue reported by @sile.

Making _SearchSpaceTransform a public API and passing it instead of just bounds will be left for future discussion and work (which I'm happy to be assigned to).

Making crossovers accept one-hot-encoded params will be left for future discussion.

Perfect! Thank you very much for this useful discussion! > @xadrianzetx @hvy @HideakiImamura

sile · 2022-01-29T00:19:22Z

Seems like workflows are stuck. Could you guys re-trigger them?

Sorry, I don't know how to resolve that.
(I could re-run the workflows for a bit old commit 2520dcf. However, I could not find workflows to be re-trigger for the latest commit via the GitHub web UI.)

@HideakiImamura Could you address this problem?

xadrianzetx · 2022-01-29T14:48:11Z

~~I think i may have to close and re-open this PR to resolve CI hangup, since pushing new commits does not seem to resolve the issue. I think I'm unable to fix the status of those jobs on my side.~~ @HideakiImamura, @sile merging master to resolve conflicts also re-triggered the jobs.

…rossover-classes

xadrianzetx · 2022-02-10T13:15:45Z

@sile, @HideakiImamura, changes discussed in #3221 (comment) are now implemented. Sorry for small delay and PTAL.

sile

LGTM 👍

HideakiImamura

Thanks for the long running PR! LGTM!

xadrianzetx · 2022-02-14T07:46:06Z

Thank you for reviews and discussions @sile, @HideakiImamura, @hvy!

xadrianzetx added 5 commits January 9, 2022 11:14

Implement BaseCrossover

32445fe

This class forms common API for all existing and future NSGA-II crossover operations.

Refactor uniform crossover into BaseCrossover subclass

2d07298

Use n_parents crossover attribute while selecting parents

87fc994

Use common crossover API in _try_crossover

5957663

Use class based crossover operations

e88ef3f

github-actions bot added the optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. label Jan 9, 2022

xadrianzetx added 8 commits January 12, 2022 18:08

Refactor BLX-alpha crossover into BLXAlphaCrossover subclass

7ebbaab

Refactor SPX crossover into SPXCrossover subclass

72e328c

Tag experimental features

0b94a6f

Use correct axis for array ops in BLX-alpha

cb4f912

Refactor SBX crossover into SBXCrossover subclass

69e3b04

Refactor vSBX crossover into VSBXCrossover subclass

ef285d0

Refactor UNDX crossover into UNDXCrossover subclass

55657e9

Use crossover classes to parametrize NSGA-II tests

4674dc4

xadrianzetx added 8 commits January 15, 2022 21:41

Fix isort

1334d0f

Reference crossover classes in NSGA-II documentation

707b320

Improve docstring formatting

7c9812e

Link crossover documentation

124ffa7

Remove crossover functions

4bd9238

Test uniform crossover with fixed rng

3b03f89

Fix mask selection logic in UniformCrossover

0e4b332

Sharp inequality is needed to follow spec

Test crossovers with fixed rng

5dcffc4

xadrianzetx marked this pull request as ready for review January 20, 2022 19:10

toshihikoyanase added the feature Change that does not break compatibility, but affects the public interfaces. label Jan 21, 2022

toshihikoyanase assigned sile and HideakiImamura Jan 21, 2022

Ignore samplers generated subtree

2520dcf

HideakiImamura reviewed Jan 27, 2022

View reviewed changes

optuna/samplers/nsga2/_crossovers/_base.py Outdated Show resolved Hide resolved

optuna/samplers/nsga2/_crossovers/_base.py Outdated Show resolved Hide resolved

tests/samplers_tests/test_nsga2.py Show resolved Hide resolved

xadrianzetx added 2 commits January 27, 2022 18:08

Clarify parents_params search space in docstring

be73c7e

Clarify search_space argument

ed8829e

sile reviewed Jan 29, 2022

View reviewed changes

xadrianzetx added 3 commits January 29, 2022 15:24

Rename nsga2 module to nsgaii to follow sampler naming

4bee896

Rename NSGAII test module to follow convention

1d1d5ee

Switch nsga2 references to nsgaii in docs

5a2145a

xadrianzetx closed this Feb 1, 2022

xadrianzetx reopened this Feb 1, 2022

xadrianzetx added 7 commits February 1, 2022 13:50

Merge branch 'master' of https://github.com/optuna/optuna into feat-c…

dcccd8b

…rossover-classes

Fix black

9108403

Fix title underline

59d5e26

Inline uniform crossover for categorical parameters

fbeb3bc

Modify BaseCrossover API to accept transformed distribution bounds

288acff

Use transformed search space instead of original in crossovers

0356ad0

Pass transformed search space to crossovers

1b958cf

Fix sphinx directive

a55ed50

sile approved these changes Feb 11, 2022

View reviewed changes

xadrianzetx changed the title ~~[RFC] Refactor NSGA-II crossover operations into classes~~ Refactor NSGA-II crossover operations into classes Feb 11, 2022

HideakiImamura approved these changes Feb 14, 2022

View reviewed changes

HideakiImamura merged commit 444695d into optuna:master Feb 14, 2022

HideakiImamura added this to the v3.0.0-b0 milestone Feb 14, 2022

xadrianzetx deleted the feat-crossover-classes branch February 14, 2022 07:45

hrntsm mentioned this pull request Aug 31, 2022

Update optuna to v3 hrntsm/Tunny#102

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor NSGA-II crossover operations into classes #3221

Refactor NSGA-II crossover operations into classes #3221

xadrianzetx commented Jan 9, 2022 •

edited

Loading

xadrianzetx commented Jan 9, 2022

codecov-commenter commented Jan 15, 2022 •

edited

Loading

toshihikoyanase commented Jan 21, 2022

sile commented Jan 21, 2022

HideakiImamura left a comment

xadrianzetx commented Jan 28, 2022

sile Jan 28, 2022

sile Jan 29, 2022

xadrianzetx Jan 29, 2022

xadrianzetx Jan 29, 2022 •

edited

Loading

sile Jan 30, 2022

xadrianzetx Feb 3, 2022

hvy Feb 4, 2022 •

edited

Loading

HideakiImamura Feb 4, 2022

xadrianzetx Feb 4, 2022 •

edited

Loading

sile Feb 4, 2022

sile commented Jan 29, 2022

xadrianzetx commented Jan 29, 2022 •

edited

Loading

xadrianzetx commented Feb 10, 2022

sile left a comment

HideakiImamura left a comment

xadrianzetx commented Feb 14, 2022

Refactor NSGA-II crossover operations into classes #3221

Refactor NSGA-II crossover operations into classes #3221

Conversation

xadrianzetx commented Jan 9, 2022 • edited Loading

Motivation

Description of the changes

xadrianzetx commented Jan 9, 2022

codecov-commenter commented Jan 15, 2022 • edited Loading

Codecov Report

toshihikoyanase commented Jan 21, 2022

sile commented Jan 21, 2022

HideakiImamura left a comment

Choose a reason for hiding this comment

xadrianzetx commented Jan 28, 2022

sile Jan 28, 2022

Choose a reason for hiding this comment

sile Jan 29, 2022

Choose a reason for hiding this comment

xadrianzetx Jan 29, 2022

Choose a reason for hiding this comment

xadrianzetx Jan 29, 2022 • edited Loading

Choose a reason for hiding this comment

sile Jan 30, 2022

Choose a reason for hiding this comment

xadrianzetx Feb 3, 2022

Choose a reason for hiding this comment

hvy Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

HideakiImamura Feb 4, 2022

Choose a reason for hiding this comment

xadrianzetx Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

sile Feb 4, 2022

Choose a reason for hiding this comment

sile commented Jan 29, 2022

xadrianzetx commented Jan 29, 2022 • edited Loading

xadrianzetx commented Feb 10, 2022

sile left a comment

Choose a reason for hiding this comment

HideakiImamura left a comment

Choose a reason for hiding this comment

xadrianzetx commented Feb 14, 2022

xadrianzetx commented Jan 9, 2022 •

edited

Loading

codecov-commenter commented Jan 15, 2022 •

edited

Loading

xadrianzetx Jan 29, 2022 •

edited

Loading

hvy Feb 4, 2022 •

edited

Loading

xadrianzetx Feb 4, 2022 •

edited

Loading

xadrianzetx commented Jan 29, 2022 •

edited

Loading