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

Adding initial version of eventified Bayesian optimization system #32

Merged
merged 5 commits into from
Oct 19, 2022
Merged

Adding initial version of eventified Bayesian optimization system #32

merged 5 commits into from
Oct 19, 2022

Conversation

shaymeister
Copy link
Collaborator

@shaymeister shaymeister commented Aug 3, 2022

Issue Number: #30 #31

Objective of pull request: Our objective is to merge the initial version of the eventified Bayesian optimization system to Lava-Optimization.

Pull request checklist

Your PR fulfills the following requirements:

  • Issue created that explains the change and why it's needed
  • Tests are part of the PR (for bug fixes / features)
  • Docs reviewed and added / updated if needed (for bug fixes / features)
  • PR conforms to Coding Conventions
  • PR applys BSD 3-clause or LGPL2.1+ Licenses to all code files
  • Lint (pyb) passes locally
  • Build tests (pyb -E unit) or (python -m unittest) passes locally

Pull request type

Please check your PR type:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation changes
  • Other (please describe):

What is the current behavior?

  • We need an initial optimization system that uses Bayesian Optimization to learn multi-dimensional black-box functions and find their global/local minima.
  • The system shall have a similar architecture to the existing QP solver via models, processes, and a solver class.

What is the new behavior?

  • We have added the initial version of the eventified Bayesian optimization system in an architecture that parallels the existing QP solver system.
  • Separate models.py and processes.py files for problems and the solver
  • There is a BayesianSolver class that abstracts away many of the lower-level details from the user.

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

I have also added the tutorial commit into this request. You will notice that it is in a regular Python script versus a Jupyter notebook. I was receiving the "attribute not found on main" error (MacOS) and it seems to be a well documented issue (lava-nc/lava#148). Once this is fixed, I will be more than happy to resubmit the tutorial as a notebook!

@awintel
Copy link
Contributor

awintel commented Aug 3, 2022

Thanks for this PR. At least @srrisbud is going to review this after his vacation in WW34. Perhaps @GaboFGuerra or @ashishrao7 want to also take a look.

@GaboFGuerra GaboFGuerra added the 1-feature New feature or request label Aug 4, 2022
@ashishrao7
Copy link
Collaborator

ashishrao7 commented Aug 12, 2022

Hi @shaymeister,

In general your code looks clean, consistent and well-documented. Good job!

Before I thoroughly review your code. I have a few general questions though.

  1. Why does your PR have separate process.py and models.py for problems. From how I understand it, problems are fed into a suitable solver. The solver runs on Loihi and therefore you have a processes.py to instantiate the processes that constitute the solver and their corresponding python models or neuron core models files in models.py or ncmodels.py. In your case, does your problem also have to run on Loihi before it can be fed into the solver (which also runs on Loihi)? Maybe what you did is right but I want to be sure that I understand what you did.

  2. This is probably trivial, but you are our first external contributor. Do you also have to include Intel corporation in the license for code that you developed and wrote? I am not sure. Maybe @GaboFGuerra or @awintel can throw some light on this.

  3. If your tutorial is not yet fully functional, it's better to have it as part of a new PR when the bug corresponding to your issue is fixed.

@shaymeister
Copy link
Collaborator Author

@ashishrao7 Thank you for the thorough response! I am more than happy to help.

  1. We have separate processes.py and models.py files for two reasons... (1) Bayesian optimization operates on a strict contract between the solver and the problem. This contract defines the parameter and objective space without any underlying knowledge of the black box function. As such, these files help define the contract between a sample problem and the solver. I like to think about these processes and models as wrappers around the user's problem. (2) These could have been included in the main processes.py and models.py files but I chose to separate them to reduce scope. Once the separate processes and models are defined, the wrapper process is passed into the Bayesian solver.

  2. I am not sure if collaborators need to add the Intel corporation license so I went ahead and added it.

  3. As it stands, the tutorials work as expected. The main issue is that the markdown documentation isn't as clear within .py files as .ipynb. We had to resort to the .py file format due to a multiprocessing issue between Jupyter and Lava.

I hope this helps answer your questions! Please let me know if there is anything else I can help you with!

Copy link
Contributor

@tim-shea tim-shea left a comment

Choose a reason for hiding this comment

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

Great contribution! Thank you very much for the effort! I left a few thoughts on design and potential tweaks to improve readability of the code.

requirements.txt Outdated Show resolved Hide resolved
src/lava/lib/optimization/solvers/bayesian/models.py Outdated Show resolved Hide resolved
src/lava/lib/optimization/solvers/bayesian/models.py Outdated Show resolved Hide resolved
src/lava/lib/optimization/solvers/bayesian/models.py Outdated Show resolved Hide resolved
src/lava/lib/optimization/solvers/bayesian/models.py Outdated Show resolved Hide resolved
src/lava/lib/optimization/solvers/bayesian/solver.py Outdated Show resolved Hide resolved
@shaymeister
Copy link
Collaborator Author

@tim-shea Thank you very much for taking the time to review the code! I just got settled into the semester so I will start addressing your comments today. I will be in touch over the next few days! 😄

@shaymeister shaymeister reopened this Oct 3, 2022
@srrisbud srrisbud merged commit 85e522c into lava-nc:main Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants