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

Add tests for the suggest method #1316

Merged
merged 22 commits into from
Mar 22, 2022
Merged

Add tests for the suggest method #1316

merged 22 commits into from
Mar 22, 2022

Conversation

teytaud
Copy link
Contributor

@teytaud teytaud commented Dec 3, 2021

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context / Related issue

We want to be sure that "suggest" works.
It turns out that it does work.

How Has This Been Tested (if it applies)

Checklist

  • The documentation is up-to-date with the changes I made.
  • I have read the CONTRIBUTING document and completed the CLA (see CLA).
  • All tests passed, and additional code has been covered with new tests.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 3, 2021
Copy link
Contributor

@jrapin jrapin left a comment

Choose a reason for hiding this comment

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

There is a lot of very redundant code, you could easily change the target depending on the case instead

nevergrad/optimization/test_optimizerlib.py Outdated Show resolved Hide resolved
nevergrad/optimization/test_optimizerlib.py Outdated Show resolved Hide resolved
@jrapin jrapin changed the title testing suggest Add tests for the suggest method Dec 6, 2021
@teytaud
Copy link
Contributor Author

teytaud commented Dec 15, 2021

There is a lot of very redundant code, you could easily change the target depending on the case instead

The different tests are distinct though. Some cases are continuous and others are discrete. Also, in some cases we just check "if the optimum has been suggested then we find it" and in other cases we check the stronger requirement "if a good suboptimal point has been provided then we find the optimum".

@teytaud
Copy link
Contributor Author

teytaud commented Dec 15, 2021

There is a lot of very redundant code, you could easily change the target depending on the case instead

The different tests are distinct though. Some cases are continuous and others are discrete. Also, in some cases we just check "if the optimum has been suggested then we find it" and in other cases we check the stronger requirement "if a good suboptimal point has been provided then we find the optimum".

I think compacting more would decrease readability.

@jrapin
Copy link
Contributor

jrapin commented Dec 17, 2021

There is a lot of very redundant code, you could easily change the target depending on the case instead

The different tests are distinct though. Some cases are continuous and others are discrete. Also, in some cases we just check "if the optimum has been suggested then we find it" and in other cases we check the stronger requirement "if a good suboptimal point has been provided then we find the optimum".

I think compacting more would decrease readability.

But increase cost of maintenance, there is too much shared code here, I am sure it's possible to be both readable and factorized

teytaud and others added 3 commits March 14, 2022 09:46
* Add architecture search for Gym (#1315)

* as

* black

* add_Test

* type_fix

* typing

* fix

* fix_bug

* fix_weird

* wtf

* fix

* fix

* fix

* black

* switch_to_choice

* mypy

* fix

* fix

* fix

* fix

* fix

* fix_type

* Adding optimization specifically for RL (#1303)

* Adding optimization specifically for RL

* fix

* fix

* Import benchmarks online for fixing dependencies (#1310)

* fix_dependencies

* fix_tests

* : missing

* fix_lint

* fix

* fix

* fix

* XPs with sparsity on Open AI Gym (#1319)

* XPs with sparsity on Open AI Gym

* Update gymexperiments.py

* Adding state of the art results on OpenAI Gym results (#1318)

* Update plotting.py

* fix

* ok_good

* fix

* Adding example for OpenAI Gym (#1320)

* testing more algorithms for permutations (#1321)

* testing more algorithms for permutations

* fix

* Adding yahdlbbbob and simplifying stuff (#1145)

* Update experiments.py

* black

* fix_case_1

* fix

* Update experiments.py

* Add Olympus benchmark (#1190)

* Add olympus function and benchmark

* remove surfaces with error

* Add noise and use average of noise free surfaces for evaluation_finction

* fix static tests

* rename olympus to olympussurfaces

* fix test_core olympus

* replace olympus with olymp in requirements

* fix olymp version in requirements

* black reformatting

* Add discrete and GaussianMixture surfaces

* Add discrete and GaussianMixture surfaces

* Move surfaces in a class variable

* no_need_for_42_because_noiseless

* nowindowsforcarraz

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* bettertest

* Add olympus emulators

* add BayesNeuralNet

* reformat code

* minor fix

* minor fix

* minor fix

* add missing package and correct seed problem

* remove unused packages

* fix silence_tensorflow version

* try after merge

* fix

* gamma_fix

* fix

* fix

* fix_naming

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* Update requirements/bench.txt

Co-authored-by: Jérémy Rapin <[email protected]>

* YEEEEEEEES_it_works

Co-authored-by: ncarraz <[email protected]>
Co-authored-by: Jérémy Rapin <[email protected]>

* Documentation for permutations (#1322)

* change sparsity in gym (#1328)

* change sparsity in gym

* fix

* Unstack ngopt (#1330)

* fix plot (#1332)

* fix plot

* black

* Fix gym: thread safety (#1338)

* fixgym

* fix

* fix

* fix

* Avoid bool check on params (#1349)

* Keep only final optimizer in portfolio (#1350)

* Fix nan in cma (#1347)

* fixnan

* fixnan

* fixnan

* fo

* Keep data in cache for CMA (#1351)

* Add installation instruction for MuJoCo (#1352)

* adding installation instruction for MuJoCo

* raise error

* better error handling

Co-authored-by: Mathurin Videau <[email protected]>

* enable_pickling for NGOpt (#1356)

* Bugfix in scrambled progressive optimization. (#1357)

* Add anisotropic progressive optimization (#1344)

* Adding anisotropic progressive optimization

* black

* Finish adding enable_pickling for all optimizers (Rescaled) (#1358)

* Residual controllers in RL. (#1359)

* Residual controllers in RL.

It's not exactly residual, it's initializing with something closer to identity. It works pretty well.

* Update multigym.py

* fix

* fix

* fix

* black

* fix

* seed

* Update test_core.py

* fix

* fix

* fix

* Bump version to 0.4.3.post10 (#1364)

* Removing an incomplete sentence from the doc (#1367)

* Fix broken CI (#1370)

* docs: add GH button in support of Ukraine  (#1369)

* Add the FAO crop model (#1343)

* aquacrop

* fix

* fix

* fix

* Update ac.py

* black

* Update experiments.py (#1361)

* fix

* Update bench.txt

* fix

* fix

* fix

* tentative_pip3

* yet_another_tentative_fi

* yet_another_tentative_fi

* fix

* fix_suffering

* desperate_try

* desperate_try

* desperate_try

* desperate_try

* fix

* desperate_try

* desperate_try

* desperate_try

* desperate_try

* fix

* Update config.yml

* fix

* Update setup.py

* Update main.txt

* fix

* Use up-to-date headers (#1371)

* Add NLOPT as a solver (#1340)

* Update version and changelog to 0.5.0 (#1372)

* Deactivate mutation test in CI (#1374)

* Reduce noise in gym (#1333)

* Reduce noise in gym

* Update multigym.py

* Add comment

* NaN robustness in Gym (#1289)

* NaN robustness in Gym

* Update nevergrad/functions/gym/multigym.py

Co-authored-by: Jérémy Rapin <[email protected]>

* fix

* fix

Co-authored-by: Jérémy Rapin <[email protected]>

* Add more models for Gym control (#1346)

* more models for Gym control

* Update gymexperiments.py

* Update multigym.py

* fix

* Update gymexperiments.py

* update moremodels (#1378)

* Bump version to 0.4.3.post10 (#1364)

* Removing an incomplete sentence from the doc (#1367)

* Fix broken CI (#1370)

* docs: add GH button in support of Ukraine  (#1369)

* Add the FAO crop model (#1343)

* aquacrop

* fix

* fix

* fix

* Update ac.py

* black

* Update experiments.py (#1361)

* fix

* Update bench.txt

* fix

* fix

* fix

* tentative_pip3

* yet_another_tentative_fi

* yet_another_tentative_fi

* fix

* fix_suffering

* desperate_try

* desperate_try

* desperate_try

* desperate_try

* fix

* desperate_try

* desperate_try

* desperate_try

* desperate_try

* fix

* Update config.yml

* fix

* Update setup.py

* Update main.txt

* fix

* Use up-to-date headers (#1371)

* Add NLOPT as a solver (#1340)

* Update version and changelog to 0.5.0 (#1372)

* Deactivate mutation test in CI (#1374)

* Reduce noise in gym (#1333)

* Reduce noise in gym

* Update multigym.py

* Add comment

Co-authored-by: Jérémy Rapin <[email protected]>
Co-authored-by: Dmitry Vinnik <[email protected]>

* fix

* fix

* im_lost

* fix

* fix

* fix

Co-authored-by: Jérémy Rapin <[email protected]>
Co-authored-by: Dmitry Vinnik <[email protected]>

* Add a conformant GP experiment (#1337)

* Adding a conformant GP experiment

Because sometimes conformant planning, in spite of being super simple, performs incredibly well.

* fix

* High-speed differential evolution (#1366)

* High-speed differential evolution

* Update base.py

* Update optimizerlib.py

* fix

* fix

* clean

* clean

* Update differentialevolution.py

* Update nevergrad/optimization/differentialevolution.py

Co-authored-by: Jérémy Rapin <[email protected]>

* Update nevergrad/optimization/differentialevolution.py

Co-authored-by: Jérémy Rapin <[email protected]>

* fi

* fix

* fix

* fix

Co-authored-by: Jérémy Rapin <[email protected]>

* cleaning

* cleaning

* cleaning

Co-authored-by: ncarraz <[email protected]>
Co-authored-by: Jérémy Rapin <[email protected]>
Co-authored-by: mathuvu <[email protected]>
Co-authored-by: Mathurin Videau <[email protected]>
Co-authored-by: Jeremy Reizenstein <[email protected]>
Co-authored-by: Dmitry Vinnik <[email protected]>
@teytaud
Copy link
Contributor Author

teytaud commented Mar 15, 2022

There is a lot of very redundant code, you could easily change the target depending on the case instead

The different tests are distinct though. Some cases are continuous and others are discrete. Also, in some cases we just check "if the optimum has been suggested then we find it" and in other cases we check the stronger requirement "if a good suboptimal point has been provided then we find the optimum".

I think compacting more would decrease readability.

But increase cost of maintenance, there is too much shared code here, I am sure it's possible to be both readable and factorized

ok for you now ?

Copy link
Contributor

@jrapin jrapin left a comment

Choose a reason for hiding this comment

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

given it's added 3 very specific tests on one particular topic, it may be better to move to a new file test_suggest.py

nevergrad/optimization/test_optimizerlib.py Outdated Show resolved Hide resolved
Comment on lines 268 to 269
def good_at_c0_suggest(r: str) -> bool:
return "NGOpt" == r # Let us check on NGOpt only for now.
Copy link
Contributor

Choose a reason for hiding this comment

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

is that really needed as a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Contributor

@jrapin jrapin left a comment

Choose a reason for hiding this comment

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

the code seems very redundant, but not sure it could be restructured better

@teytaud
Copy link
Contributor Author

teytaud commented Mar 18, 2022

the code seems very redundant, but not sure it could be restructured better

is it ok for you now ? i refactored cycling.py but not the underlying computations: i assume that original authors would be lost if I changed the deep stuff.

Copy link
Contributor

@jrapin jrapin left a comment

Choose a reason for hiding this comment

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

I would still move all suggestions tests to one test_suggest.py file because we start to have quite a few of them, and test_optimizerlib is too big

@teytaud teytaud merged commit 213bbc8 into main Mar 22, 2022
@teytaud teytaud deleted the suggest branch March 22, 2022 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants