msmbuilder / osprey

🦅Hyperparameter optimization for machine learning pipelines 🦅
http://msmbuilder.org/osprey
Apache License 2.0
74 stars 26 forks source link

Refactor #247

Closed RobertArbon closed 5 years ago

RobertArbon commented 6 years ago

I've refactored the code so that there are now four strategies:

The GP strategy is now a type of surrogate function. There is also now a general Acquisition function class.

I've not added any more tests at the moment as I'd like some guidance on that if possible.

I'm starting a PL but it doesn't seem to pass my own Travis CI - it fails on three tests which require the fs_peptide data set. See here. I keep getting Import Error msmbuilder.io.sampling No module named 'sklearn.grid_search'

The three tests that are failing are:

  1. test_cli_worker_and_dump.test_msmbuilder_skeleton()
  2. test_cli_worker_and_dump.test_msmb_feat_select_skeleton()
  3. test_dataset_loader.test_MDTrajDatasetLoader_1()
  4. test_fit_estimator.test_1()

All the tests are passed when running the same test suite on my local computer however. The last one I think is because you need to download the data files properly.

UPDATE:

The problem for 1 - 2 is an import in msmbuilder. I've made a pull request there to fix the problem. In the mean time I've fixed that by making wrapping the imports in a try/except block. This means that these tests don't get loaded at the moment.

The fix for 3 is to actually download the data in the test in the same way as the other fs_peptide tests are downloaded. The import will be in a try/except block.

The fix for 4 was to change to the sklean 0.20 api for the GridSearchCV object.

There is a mystery as to why test_dataset_loader.test_MSMBuilderDatasetLoader_1() passes the test though - apparently there is no problem in importing msmbuilder.dataset

cxhernandez commented 5 years ago

Thanks @RobertArbon! This looks great. Sorry for reviewing these changes so late— has been hard to find the time to get the master branch back into a state where PRs can be merged.

If you don't mind, I'll remove the pyemma and SobolSearch additions (feel free to add them back in a future PR) so as to better organize the focus of this PR a bit.