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 MOFA #864

Merged
merged 28 commits into from
May 25, 2022
Merged

Add MOFA #864

merged 28 commits into from
May 25, 2022

Conversation

wagnersj
Copy link
Contributor

Description

Adds the MOFA algorithm to Orion.

Tests

  • I added corresponding tests for bug fixes and new features. If possible, the tests fail without the changes
  • All new and existing tests are passing ($ tox -e py38; replace 38 by your Python version if necessary)

Documentation

  • I have updated the relevant documentation related to my changes

Quality

  • I have read the CONTRIBUTING doc
  • My commits messages follow this format
  • My code follows the style guidelines ($ tox -e lint)

Copy link
Collaborator

@lebrice lebrice left a comment

Choose a reason for hiding this comment

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

👍

src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/sampler.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/sampler.py Outdated Show resolved Hide resolved
tests/unittests/algo/mofa/test_mofa.py Show resolved Hide resolved
wagnersj and others added 6 commits March 29, 2022 11:35
Co-authored-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Co-authored-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Co-authored-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Co-authored-by: Fabrice Normandin <fabrice.normandin@gmail.com>
src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
src/orion/algo/mofa/mofa.py Outdated Show resolved Hide resolved
@wagnersj
Copy link
Contributor Author

As of ee7139c, all tests on MOFA pass except one: test_is_done_max_trials(). The problem has been traced to trials being returned by MOFA.suggest() that the test rig views as duplicates. While MOFA.suggest() does check for duplicates, it does not handle rounding of parameter values for Space dimensions that are wrapped by Precision, but the test rig does via TransformedSpace. Hence, when MOFA.suggest() checks for duplicates, it has more digits in Real values to compare against, while the test rig may see a duplicate after rounding to less digits. Is there an elegant way to handle this within MOFA.suggest() by transforming the space of the trails to do the rounding prior to registering the trial?

@bouthilx
Copy link
Member

bouthilx commented Apr 1, 2022

About test_is_done_max_trials(), one solution would be to avoid testing for max_trials within MOFA since it is already done within the wrapper, and the wrapper holds the ground truth in terms of how many trials have been actually executed and completed. That would mean removing the part or super().is_done in MOFA.is_done property.

@wagnersj
Copy link
Contributor Author

wagnersj commented Apr 1, 2022

About test_is_done_max_trials(), MOFA does not do checks on max_trials. I did try @bouthilx 's suggestion to remove the super().is_done check in the is_done() function, but that does not solve the problem (actually causes another test to fail).

The test_is_done_max_trials() fails with the following message: RuntimeError: Algorithm cannot sample more than 102 trials. Is it normal?. This happens because the BaseAlgoTests.force_observe() function call within test_is_done_max_trials() gets empty trials from the suggest()function and eventually sees 5 empty trial returns as a sign of a failure. The empty trials are not being returned by MOFA.suggest() but by SpaceTransformAlgoWrapper.suggest().

I've investigated deeper. The problem is that MOFA and SpaceTransformAlgoWrapper are both checking for duplicates. I think the best way to handle this is to remove the checks in MOFA and let SpaceTransformAlgoWrapper do it.

@lebrice
Copy link
Collaborator

lebrice commented Apr 26, 2022

The test that wasn't working should now work: The force_observe will always observe the required number of trials.

@bouthilx
Copy link
Member

bouthilx commented May 4, 2022

Reference to pandas could be added here: https://github.com/Epistimio/orion/blob/develop/docs/src/conf.py#L254

@wagnersj
Copy link
Contributor Author

All tests on this PR are passing except for the Python 3.7 tests. This is due to MOFA needing scipy v1.8.0 which is only supported in Python >= 3.8. Are we still planning on supporting Python 3.7? If so, is there a way to for the CI to skip tests on MOFA if Python 3.7 tests are being run?

@bouthilx
Copy link
Member

Tests could be skipped like this: https://docs.pytest.org/en/7.1.x/how-to/skipping.html#id1. It would be good to have as well a clear error message when correct scipy version cannot be imported.

@bouthilx
Copy link
Member

It looks like its failing when the extra dependencies of the other libraries are not installed. Could it be depending on a dependency that is not in the core dependencies but is in the extra of another algorithm?

@wagnersj
Copy link
Contributor Author

It looks like the problem in build #1716 is due to changes merged in PR #913 in the force_observe() method. There is a failure in a call to the get_num() function of the BaseAlgoTest class.

TestPhase("3rd-run", n_trials * 2, "space.sample"),
]

def get_num(self, num):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def get_num(self, num):
@classmethod
def get_num(cls, num):

This should solve it @wagnersj

@lebrice
Copy link
Collaborator

lebrice commented May 18, 2022

Hey @wagnersj : There's a simple conflict to solve in setup.py, and you can just apply the fix I mention above for the get_num:

I test this locally, checked out develop, merged your branch, fixed the simple conflict in setup.py, and ran the tests for mofa in python 3.9: Everything works!

@bouthilx bouthilx merged commit f466223 into Epistimio:develop May 25, 2022
@bouthilx bouthilx added the feature Introduces a new feature label May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants