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

Reorganize simulation lookup #265

Merged
merged 10 commits into from
Jul 8, 2024
Merged

Reorganize simulation lookup #265

merged 10 commits into from
Jul 8, 2024

Conversation

AdrianSosic
Copy link
Collaborator

@AdrianSosic AdrianSosic commented Jun 11, 2024

This PR is another step toward a cleaned simulation package:

  • Decouples the lookup mechanism from Campaign so that it can be flexibly used without having to set up a campaign object first (e.g. in tests)
  • Splits up the monolithic lookup function into different cases
  • Rewrites the docstrings and activates doctests
  • Moves simulation-related utilities to the simulation package

Copy link
Collaborator Author

@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, just wanted to let you know that I've rebase the PR on top of #266, which finally cleans up our optional imports and also solves the problem that remained in this PR. So let's review the other PR first 😃

@AdrianSosic AdrianSosic force-pushed the refactor/lookup branch 2 times, most recently from d227c24 to 24c282b Compare June 19, 2024 07:32
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
baybe/simulation/_imputation.py Show resolved Hide resolved
baybe/simulation/_imputation.py Show resolved Hide resolved
baybe/simulation/_imputation.py Show resolved Hide resolved
pytest.ini Outdated Show resolved Hide resolved
baybe/simulation/core.py Show resolved Hide resolved
baybe/simulation/lookup.py Show resolved Hide resolved
baybe/simulation/lookup.py Show resolved Hide resolved
baybe/simulation/lookup.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.

First round of comments

CHANGELOG.md Outdated Show resolved Hide resolved
baybe/simulation/_imputation.py Show resolved Hide resolved
baybe/simulation/_imputation.py Show resolved Hide resolved
baybe/simulation/_imputation.py Show resolved Hide resolved
baybe/simulation/lookup.py Show resolved Hide resolved
baybe/simulation/lookup.py Outdated Show resolved Hide resolved
baybe/simulation/lookup.py Show resolved Hide resolved
baybe/simulation/lookup.py Show resolved Hide resolved
baybe/simulation/lookup.py Show resolved Hide resolved
baybe/simulation/lookup.py Show resolved Hide resolved
baybe/simulation/lookup.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.

Seems like most of my comments were in fact not relevant since they dealt with old code, hence approve

@AdrianSosic AdrianSosic force-pushed the refactor/lookup branch 5 times, most recently from b385912 to 6fe6e50 Compare July 3, 2024 20:13
pytest.ini Show resolved Hide resolved
@AdrianSosic AdrianSosic merged commit fd425cc into main Jul 8, 2024
10 checks passed
@AdrianSosic AdrianSosic deleted the refactor/lookup branch July 8, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants