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

Enable Pending Points #319

Merged
merged 27 commits into from
Sep 6, 2024
Merged

Enable Pending Points #319

merged 27 commits into from
Sep 6, 2024

Conversation

Scienfitz
Copy link
Collaborator

@Scienfitz Scienfitz commented Jul 21, 2024

This is an important step towards enabling asynchronous workflows, by allowing users to identify pending_experiments

Added:

  • .recommend from Recommender and Campaign gets a new argument pending_experiments
  • All pure recommenders now have the option allow_recommending_pending_experiments. If set to False, pending experiments will be excluded from the candidates (this only holds for exact matches to the searchspace and is only relevant in purely discrete search spaces)
  • For Bayesian recommenders, additionally, the info is passed down to the acquisition function as X_pending. This only works for mc-acqfs and an error is thrown in case of an incompatible acqf
  • A test verifies that a situation with subsequent .recommend calls and marking points as pending in the second call (keeping the randoms seed same for both calls) does not produce duplicated recommendations
  • A minimal userguide to make users aware and give an example of how to use it
  • Found that it is rather easy to make fixtures available to doctests and did that for the new guide, but did not check whether any other previous ~~~ code block could benefit from that
  • README entry mentioning the asynchronous capabilities

NOT added:

  • the Campaign does not do any further handling of pending experiments, eg like storing them and so on. Providing pending_experiments also invalidates cached recommendations

@Scienfitz Scienfitz added the new feature New functionality label Jul 21, 2024
@Scienfitz Scienfitz self-assigned this Jul 21, 2024
@Scienfitz Scienfitz force-pushed the feature/pending_points branch 2 times, most recently from 74101aa to ca866e1 Compare July 25, 2024 12:29
Copy link
Collaborator

@AdrianSosic AdrianSosic left a comment

Choose a reason for hiding this comment

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

Hi @Scienfitz, thanks a lot for tackling the pending measurements issue. Here a first batch of comments, mostly regarding the user guide (thanks for writing this as well, btw). Will how have a look at the code

README.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

There are no tests/examples of adding measurements for points that were declared pending, and I think this is absolutely necessary.
Other than that this looks really good :)

CHANGELOG.md Outdated Show resolved Hide resolved
baybe/campaign.py Outdated Show resolved Hide resolved
baybe/acquisition/base.py Show resolved Hide resolved
docs/userguide/async.md Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/userguide.md Outdated Show resolved Hide resolved
tests/docs/test_docs.py Show resolved Hide resolved
tests/test_pending_points.py Outdated Show resolved Hide resolved
tests/test_pending_points.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@AdrianSosic AdrianSosic left a comment

Choose a reason for hiding this comment

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

Hi @Scienfitz, again, thanks a lot for the PR. Have made some suggestions but the big elephant in the room is the decision on how to handle the points in the first place. Have mentioned one idea in a thread.

docs/userguide/userguide.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
baybe/searchspace/discrete.py Outdated Show resolved Hide resolved
baybe/recommenders/pure/base.py Outdated Show resolved Hide resolved
baybe/recommenders/pure/bayesian/base.py Outdated Show resolved Hide resolved
baybe/recommenders/pure/nonpredictive/base.py Outdated Show resolved Hide resolved
baybe/recommenders/pure/nonpredictive/base.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

Re-reviewing this PR now solved my confusion regarding the adding of pending points. All of my comments are only minor or already raised by Adrian, hence approving.

docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
# Conflicts:
#	CHANGELOG.md
#	baybe/acquisition/base.py
#	baybe/campaign.py
#	baybe/recommenders/naive.py
#	baybe/recommenders/pure/base.py
#	baybe/recommenders/pure/bayesian/base.py
#	baybe/recommenders/pure/nonpredictive/clustering.py
#	baybe/recommenders/pure/nonpredictive/sampling.py
#	baybe/searchspace/continuous.py
#	baybe/searchspace/discrete.py
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

User guide is much clearer now imo and I am happy with it.

docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
docs/userguide/async.md Outdated Show resolved Hide resolved
baybe/recommenders/pure/nonpredictive/base.py Outdated Show resolved Hide resolved
baybe/recommenders/pure/nonpredictive/base.py Show resolved Hide resolved
baybe/recommenders/pure/base.py Outdated Show resolved Hide resolved
baybe/recommenders/base.py Outdated Show resolved Hide resolved
baybe/acquisition/base.py Show resolved Hide resolved
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
# Conflicts:
#	CHANGELOG.md
#	README.md
#	baybe/acquisition/base.py
#	baybe/exceptions.py
#	baybe/recommenders/pure/bayesian/botorch.py
#	tests/conftest.py
@Scienfitz Scienfitz merged commit ac1fa4d into main Sep 6, 2024
11 checks passed
@Scienfitz Scienfitz deleted the feature/pending_points branch September 6, 2024 15:18
AdrianSosic added a commit that referenced this pull request Oct 22, 2024
Was removed in PR #332 (commit 6371d97) but then mistakenly brought back in PR #319 (commit ac1fa4d)
AdrianSosic added a commit that referenced this pull request Oct 24, 2024
Was removed in PR #332 (commit 6371d97)
but then mistakenly brought back in PR #319 (commit
ac1fa4d)
fabianliebig pushed a commit that referenced this pull request Nov 8, 2024
Was removed in PR #332 (commit 6371d97) but then mistakenly brought back in PR #319 (commit ac1fa4d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants