facebookresearch / nevergrad

A Python toolbox for performing gradient-free optimization
https://facebookresearch.github.io/nevergrad/
MIT License
3.98k stars 356 forks source link

Recurrent tasks - Good first issue #409

Open jrapin opened 4 years ago

jrapin commented 4 years ago

The following are tasks that should be performed regularly and can each be good first issues for anyone willing to start playing with the codebase. Let us know here if you want to tackle one of these, and we'll be happy to help if you need it!

Many of those tasks are based on the unit tests. The latest tests run on CircleCI are visible here. You can click on the latest build to view one report. Unit tests can be executed with pytest nevergrad --durations=20 --cov=nevergrad

On a side note, feel free to join the Facebook's Nevergrad Users group.

Remove warnings in tests

Check the warnings on the [all] Run pytest tab of CircleCI or after running pytest yourself (see above). Removing the warnings can mean updating the code to use new methods, or just filtering them out if the warning is to be expected.

Improve coverage

Check the coverage report at the end of the [all] Run pytest tab of CircleCI or after running pytest yourself (see above). Files with low coverage ratio probably need more unit tests.

Fasten unit tests

Check the slowest unit tests at the very end of the [all] Run pytest tab of CircleCI or after running pytest yourself (see above). Tests which take more that 5s should be essential or have a good reason to be that slow. Consider testing the same thing differently if it can make testing faster (eg: mocking some part of the function, avoiding repetitions etc)

Enable strict type checking

If running mypy --implicit-reexport --strict --ignore-missing-imports nevergrad returns errors, then we should fix them. Most of the time this means adding type hints, but sometimes this may also mean adding # type: ignore because it would be too complicated (and unnecessary) to do better (eg: many numpy functions are not typed, so we must enforce the type even though mypy complains). Also, some parts of the code are not type checked yet (see mypy.ini), and could be updated.

sashank27 commented 4 years ago

@jrapin I would like to work on some of these tasks.

jrapin commented 4 years ago

Hi @sashank27 , happy to have you onboard ;) Currently I see a few options:

  1. Investigate the capi_return is NULL. Call-back cb_calcfc_in__cobyla__user__routines failed. that appears when running the tests (more specifically pytest nevergrad/optimization/test_optimizerlib.py::test_optimizers_recommendation). Given the text, I expect that this is due to Cobyla: https://github.com/facebookresearch/nevergrad/blob/b12878c5309f8c2d88d18fec7ee9c1637683ba9b/nevergrad/optimization/recastlib.py#L88-L89 That task would require understanding how nevergrad interfaces with scipy (which implements Cobyla) using threading (this is a bit messy). You would need to either find why this message appears, and either find a way to fix the failure if it makes sense, or hide the message if this is expected.

  2. Fix the warning when running pytest "nevergrad/benchmark/test_experiments.py::test_experiments_registry[mlda]":

    /home/circleci/repo/nevergrad/functions/mlda/problems.py:181: RuntimeWarning: invalid value encountered in true_divide
    raw_data /= np.std(raw_data, axis=0, keepdims=True)

    This part of the code implements test cases which help us evaluate the optimizers. The "MLDA" test case in particular requires downloading data, which we cannot do during the tests, so this part is mocked in the tests: https://github.com/facebookresearch/nevergrad/blob/b12878c5309f8c2d88d18fec7ee9c1637683ba9b/nevergrad/benchmark/test_experiments.py#L22 Then again, I'm not sure that's the root cause, that would be the thing to investigate, and find a way to actually solve it if possible (not juste hide the message)

  3. On nevergrad/functions/powersystems/core.py the coverage is quite low, I think this is due to the "plot" function which is not tested. While this is not very important because it's not very used, maybe a very dumb test which mocks plt (using Mock(spec=plt) or something like this) would at least make sure there is nothing stupid in the method, and will prevent us from breaking it eventually.

You can choose any of these if you are interested, or if you have other ideas let me know.

mgawlinska commented 4 years ago

Hi @jrapin, I could take the first one as my first issue. It looks like a good starting point to get familiar with the code. However I will need some time for that, would it be okey?

jrapin commented 4 years ago

@mgawlinska for now nobody has planned on solving it, so no problem, and there is no hurry so you can take your time (just let us know if you change your mind so we can put someone else on it ;) )

mgawlinska commented 4 years ago

Cool, thank you for answering so quickly. I will let you know if anything changes. For now I will start to work on this.

kwonhur commented 4 years ago

@jrapin Hi, are you still looking to resolve these issues? Can I work on the third issue if so? Thanks

jrapin commented 4 years ago

Hi @kwonhur , yes still looking and third issue is still free to grab for now ;)