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

Refactor Strategies to Recommenders #146

Merged
merged 18 commits into from
Feb 27, 2024
Merged

Conversation

Scienfitz
Copy link
Collaborator

  • move strategies to recommender module
  • restructure recommender module to have .meta and .pure for subpackages
  • new class MetaRecommender from which old strategies now derive, their name has changed from <xyz>Strategy to <xyz>MetaRecommender
  • deprecations for old strategies
  • rename Recommender to PureRecommender in analogy to MetaRecommender
  • rename NaiveHybridRecommender to NaiveHybridSpaceRecommender
  • refactor tests
  • refactor examples
  • reduce transfer learning example parameters during SMOKE_TEST

NOT included

  • Followup PR1: MyPy activation for recommenders
  • Followup PR2: user guide files in this PR can be mostly ignored, I have mainly fixed references in examples and user guides, but also their structure needs an overhaul (eg strategies don't exist anymore)

@Scienfitz Scienfitz added documentation Improvements or additions to documentation tests Related to testing. labels Feb 27, 2024
@Scienfitz Scienfitz self-assigned this Feb 27, 2024
@Scienfitz
Copy link
Collaborator Author

@AVHopp unfortunately a lot of files have changed event hough this is just a restructuring/renaming, but we need to prioritize this PR

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.

Main things I spotted:

  • At several places, we actually need the word strategy, e.g. when speaking about the abstract concept of a strategy ("DoE strategy") instead of the actual object.
  • In hypothesis related files, we also need to keep the word strategy.
  • There are some places where I'd prefer some more explanation regarding the new naming. This is nothing too big, with the exception of the "strategy" user guide where we should add some note saying that the names do not match currently although the functionality remains as described.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
baybe/campaign.py Outdated Show resolved Hide resolved
baybe/campaign.py Outdated Show resolved Hide resolved
baybe/parameters/base.py Outdated Show resolved Hide resolved
baybe/simulation.py Outdated Show resolved Hide resolved
baybe/simulation.py Outdated Show resolved Hide resolved
docs/userguide/strategies.md Show resolved Hide resolved
tests/hypothesis_strategies/parameters.py Outdated Show resolved Hide resolved
tests/test_deprecations.py Show resolved Hide resolved
@Scienfitz Scienfitz force-pushed the refactor/strats_to_recommenders branch from 6700e6c to 56b6a5e Compare February 27, 2024 10:36
@Scienfitz Scienfitz force-pushed the refactor/strats_to_recommenders branch from 56b6a5e to 9bd8234 Compare February 27, 2024 11:11
@Scienfitz Scienfitz mentioned this pull request Feb 27, 2024
tests/test_deprecations.py 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.

Just one minor question, other than that this is gucci now. Thanks for all of the tedious work :)

@Scienfitz Scienfitz merged commit 35a23f4 into main Feb 27, 2024
10 checks passed
@Scienfitz Scienfitz deleted the refactor/strats_to_recommenders branch February 27, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation tests Related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants