Completely remove auxiliary methods in models and separate them to their own functions #481
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
c8df81e to
513c5bf
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
513c5bf to
8b0be8b
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
8b0be8b to
404c23e
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
404c23e to
3501465
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
3501465 to
0737eb0
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
0737eb0 to
3eeb6b5
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
3eeb6b5 to
97d0ed6
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
Summary: The test_finish_criteria test uses the MonotonicRejectionGP alongside many mocks. This seems really brittle meaning that it fails when very subtle things changes in the model (and is seemingly impossible to debug and maintain). The min_post_range finish condition actually cares about the values of the model so we move this out of that test into its own acutally using a model we fit (GPClassificationModel) so that we can test it easier. Differential Revision: D67066880
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
force-pushed
the
export-D67118446
branch
from
97d0ed6 to
6773e2e
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 14, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Reviewed By: crasanders Differential Revision: D67118446
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 14, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Reviewed By: crasanders Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
6773e2e to
d01b077
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 14, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Reviewed By: crasanders Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
d01b077 to
0ad368d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 14, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Reviewed By: crasanders Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
0ad368d to
728269c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 16, 2024
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Reviewed By: crasanders Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
728269c to
9019c0a
Compare
…ncy (facebookresearch#475) Summary: Pull Request resolved: facebookresearch#475 #### PR Description: This PR builds upon the changes introduced in facebookresearch#467 by fully removing the usage of lower and upper bounds within all models. ## Key updates include: - **Removal of Bounds and Dimension from Models:** All `lb`, `ub`, and `dim` parameters have been fully removed from all models. The responsibility for computing inducing points is now entirely managed by allocators, ensuring a cleaner and more modular design. - **Dependency Shift to Allocators:** All model dependencies on dimensions (e.g., for computing the `covar_module`) are now managed through the `dim` property of the allocator object. This centralizes the handling of bounds and dimensions in the allocator system. - **Updated Allocators Functionality:** Allocators like `AutoAllocator` and `KMeansAllocator` now optionally take bounds. Bounds are essential in scenarios where no input data is provided, as they ensure fallback to the `DummyAllocator` (e.g., during initialization from configuration). - **Tests:** - All tests have been updated to make use of the allocator object as a requirement. - Most tests pass bounds to the allocator, while others initialize bounds via configuration. - Tests now fully align with the updated model-allocator interaction and function as expected. Let me know if any changes are needed! :) Pull Request resolved: facebookresearch#468 Differential Revision: D66968212
Summary: Move the inducing point allocator classes to its own directory under models and split each allocator out to their own files. Modified imports to match the refactor and cleaned up imports around them. Differential Revision: D67044580
…facebookresearch#478) Summary: Pull Request resolved: facebookresearch#478 Inducing point allocator classes had extra methods and attributes that were not necessary or misnamed. These have been cleaned up in the BaseAllocator class and all of its children are similarly updated. The dummy allocator is no longer needed, instead, the allocators can just make dummy points based on this dimensionality. This sets the last_allocator_used attribute to be None to signify no actual allocator was used in creating the last set of points. This also requires all allocators to know its dimensionality at least as it is used by the dummy allocator when there's no inputs. All models that use an allocator now initializes the allocators outside as there's no trivial default for allocators anymore. Differential Revision: D67059839
Summary: The init of GPClassification was saving exta atttributes unnecessarily as well as getting largely disorganized in the refactors. We reorganize GPClassificationModel's init and its children. Differential Revision: D67104228
Summary: Unnecessary utility function replaced by just calling the Allocator class method. This diff removes using strings to select allocators. Differential Revision: D67068021
…eir own functions (facebookresearch#481) Summary: Pull Request resolved: facebookresearch#481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Reviewed By: crasanders Differential Revision: D67118446
JasonKChow
force-pushed
the
export-D67118446
branch
from
9019c0a to
fc501ad
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67118446 |
|
This pull request has been merged in 208d19f. |
JasonKChow
added a commit
that referenced
this pull request
Dec 18, 2024
…eir own functions (#481) Summary: Pull Request resolved: #481 Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy. Reviewed By: crasanders Differential Revision: D67118446 fbshipit-source-id: fde0f7e4fcabb2df9028f3eecca4d0d586c5731a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: Methods like get_min, inv_query, and dim_grid have been completely removed from models and made into utility functions. Strategy used to call these now call the utility functions. The functions are better encapsulated such that they work exactly the same way whether they're being called separately or from within a strategy.
Differential Revision: D67118446