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

Implement ConfigureableMixin in the main branch and refactor #377

Open
crasanders opened this issue Sep 8, 2024 · 0 comments
Open

Implement ConfigureableMixin in the main branch and refactor #377

crasanders opened this issue Sep 8, 2024 · 0 comments

Comments

@crasanders
Copy link
Contributor

The ax branch implements a ConfigureableMixin class: https://github.com/facebookresearch/aepsych/blob/ax-backend/aepsych/config.py#L360

This class allows any other AEPsych classes that implement from_config to fetch config arguments in an object-oriented way, allowing code re-use between super- and sub-classes. Implementing this in the main branch. See this PR for an example: #376
facebook-github-bot pushed a commit that referenced this issue Dec 5, 2024
@yalsaffar @facebook-github-bot
Refactor Inducing Point Selection to Use Allocator Classes for Enhanc…
32af5c5 
…ed Flexibility (#377) (#435)

Summary:
# Description
This PR proposes a solution to issue #378 . Although the implementation is fully functional and tested, it is presented as an overall proposal, open for discussion and potential refinement.

### Key Changes
- **Refactor of `select_inducing_points` Function**:
  - **Previous Implementation**: Accepted `method` as a string (`"pivoted_chol"`, `"kmeans++"`, `"auto"`, `"sobol"`) and used conditional logic to select inducing points.
  - **New Implementation**: Now accepts an `InducingPointAllocator` instance, with `AutoAllocator` as the default if `allocator` is `None`. This approach allows users to directly pass allocator instances, aligning with the issue’s goal to enable flexible use of custom allocators like `GreedyImprovementReduction`.

- **New `inducing_point_allocators.py` File**:
  - Introduces classes `SobolAllocator`, `KMeansAllocator`, and `AutoAllocator`, all implementing the `InducingPointAllocator` interface from Botorch. This modularizes allocator logic, moving it out of `select_inducing_points` while following the established base class structure.

- **Modifications to Models and Example Files**:
  - Updated `gp_classification.py`, `monotonic_projection_gp.py`, `monotonic_rejection_gp.py`, `semi_p.py`, and `example_problems.py` to handle allocator class instances rather than string-based methods, improving overall consistency.
  - Added imports for the new allocator classes in `__init__.py` for cross-codebase accessibility.

- **Updated Tests**:
  - Adjusted tests in `test_semi_p.py`, `test_utils.py`, and `test_config.py` to work with allocator classes instead of the previous string-based structure.

### Additional Notes
This PR preserves most of the existing logic in `select_inducing_points` to keep changes minimal. I know further work is needed to confirm compatibility with additional Botorch allocators and to support advanced configurations using `from_config` for custom allocator setups. I’d love to hear your feedback on the overall approach before moving forward with these additional refinements.

Pull Request resolved: #435

Reviewed By: crasanders

Differential Revision: D65451912

Pulled By: JasonKChow

fbshipit-source-id: e0529e545e428ad94ef965cc9d642577cbeb2777
JasonKChow pushed a commit that referenced this issue Dec 18, 2024
@yalsaffar @JasonKChow
Refactor Inducing Point Selection to Use Allocator Classes for Enhanc…
104b266 
…ed Flexibility (#377) (#435)

Summary:
# Description
This PR proposes a solution to issue #378 . Although the implementation is fully functional and tested, it is presented as an overall proposal, open for discussion and potential refinement.

### Key Changes
- **Refactor of `select_inducing_points` Function**:
  - **Previous Implementation**: Accepted `method` as a string (`"pivoted_chol"`, `"kmeans++"`, `"auto"`, `"sobol"`) and used conditional logic to select inducing points.
  - **New Implementation**: Now accepts an `InducingPointAllocator` instance, with `AutoAllocator` as the default if `allocator` is `None`. This approach allows users to directly pass allocator instances, aligning with the issue’s goal to enable flexible use of custom allocators like `GreedyImprovementReduction`.

- **New `inducing_point_allocators.py` File**:
  - Introduces classes `SobolAllocator`, `KMeansAllocator`, and `AutoAllocator`, all implementing the `InducingPointAllocator` interface from Botorch. This modularizes allocator logic, moving it out of `select_inducing_points` while following the established base class structure.

- **Modifications to Models and Example Files**:
  - Updated `gp_classification.py`, `monotonic_projection_gp.py`, `monotonic_rejection_gp.py`, `semi_p.py`, and `example_problems.py` to handle allocator class instances rather than string-based methods, improving overall consistency.
  - Added imports for the new allocator classes in `__init__.py` for cross-codebase accessibility.

- **Updated Tests**:
  - Adjusted tests in `test_semi_p.py`, `test_utils.py`, and `test_config.py` to work with allocator classes instead of the previous string-based structure.

### Additional Notes
This PR preserves most of the existing logic in `select_inducing_points` to keep changes minimal. I know further work is needed to confirm compatibility with additional Botorch allocators and to support advanced configurations using `from_config` for custom allocator setups. I’d love to hear your feedback on the overall approach before moving forward with these additional refinements.

Pull Request resolved: #435

Reviewed By: crasanders

Differential Revision: D65451912

Pulled By: JasonKChow

fbshipit-source-id: e0529e545e428ad94ef965cc9d642577cbeb2777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@crasanders