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.

Sign up for GitHub

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

Jump to bottom

Restructure the BoTorch interface #23

Merged
merged 10 commits into from
Apr 24, 2023
Merged

Restructure the BoTorch interface #23

merged 10 commits into from
Apr 24, 2023

Conversation

thomaswmorris
Copy link
Contributor

I restructured and renamed a bunch of stuff, with the philosophy that:

  • The normalization of the inputs and targets happens in models.py, so that all references to "params" and "X" have units (e.g. mm for a motor) and are never normalized, which makes things a lot less confusing
  • The Bayesian agent shouldn't have any torch in it, and just asks a model from models.py about the Gaussian Process.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

mrakitin
mrakitin reviewed Apr 22, 2023
View reviewed changes
Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

@thomaswmorris, I think it's close to completion, however, needs some work. I added a few suggestions below.

Also, can the notebook with the Himmelblau test function be improved? Right now it does not uncover all 4 optimum values:

image

Maybe having a reliable random seed and an increased number of iterations can help?

bloptools/experiments/tests/__init__.py Outdated Show resolved Hide resolved
bloptools/experiments/tests/__init__.py Outdated Show resolved Hide resolved
bloptools/experiments/tests/__init__.py Outdated Show resolved Hide resolved
bloptools/tests/test_bayesian_shadow.py
from sirepo_bluesky.sirepo_ophyd import create_classes

from bloptools.bo import BayesianOptimizationAgent
from bloptools.experiments.shadow import tes


@pytest.mark.shadow
Copy link
Member

Choose a reason for hiding this comment

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

Are those markers used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sometimes use them to exclude sirepo tests, I can split up the tests into sirepo and non-sirepo

Copy link
Member

Choose a reason for hiding this comment

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

I remember the markers need to be declared in the pytest.ini file at the top of the repo.

Example 1: https://github.com/NSLS-II/sirepo-bluesky/blob/main/pytest.ini.
Example 2: https://github.com/Synchrotron-Radiation-Workshop/SRW/blob/reorg/clients/python/pytest.ini

mrakitin and others added 5 commits April 23, 2023 12:58
@mrakitin
Fix the device generation funciton
25523db
@thomaswmorris @mrakitin
Update bloptools/experiments/tests/__init__.py
21ef5c0 
Co-authored-by: Max Rakitin <mrakitin@users.noreply.github.com>
@thomaswmorris @mrakitin
Update bloptools/experiments/tests/__init__.py
e222964 
Co-authored-by: Max Rakitin <mrakitin@users.noreply.github.com>
@thomaswmorris
notebook tweaks
f9e582f
@thomaswmorris
merge
991a23f
@thomaswmorris
Copy link
Contributor Author

@thomaswmorris, I think it's close to completion, however, needs some work. I added a few suggestions below.

Also, can the notebook with the Himmelblau test function be improved? Right now it does not uncover all 4 optimum values:

image

Maybe having a reliable random seed and an increased number of iterations can help?

I added more iterations

mrakitin
mrakitin reviewed Apr 24, 2023
View reviewed changes
examples/prepare_chx_shadow.py

connection = SirepoBluesky("http://localhost:8000")

data, schema = connection.auth("shadow", "I1Flcbdw")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this simulation exists in the database for Sirepo started with the action from the sirepo-bluesky repo. We can add it if needed. Was it converted from the HXV1JQ5c SRW simulation (from this list) to the Shadow simulation?

Why not to use the existing TES Shadow simulation?

Copy link
Member

Choose a reason for hiding this comment

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

I think it can be resolved in a separate PR.

mrakitin
mrakitin approved these changes Apr 24, 2023
View reviewed changes
Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

I like the result of optimization much better now!
image

This was referenced Apr 24, 2023
Open
Closed
@mrakitin
Copy link
Member

I've opened 2 issues to cover non-blocking requests raised during this PR review:

@thomaswmorris, please address them when you have time. I am going to merge this PR.

@mrakitin mrakitin merged commit 6b9ee62 into NSLS-II:main Apr 24, 2023
@thomaswmorris thomaswmorris deleted the normalize branch November 4, 2023 14:32
thomaswmorris pushed a commit to thomaswmorris/blop that referenced this pull request Jul 15, 2024
@mrakitin
Merge pull request NSLS-II#23 from thomaswmorris/normalize
777f4ff 
Restructure the BoTorch interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrakitin mrakitin mrakitin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomaswmorris @mrakitin