Refactor inducing point allocator classes to only need what they need #478
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: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 11, 2024
…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
JasonKChow
force-pushed
the
export-D67059839
branch
from
8d23e3a to
ebf6c64
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 11, 2024
…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
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
force-pushed
the
export-D67059839
branch
from
ebf6c64 to
07bb7b1
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 11, 2024
…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
JasonKChow
force-pushed
the
export-D67059839
branch
from
07bb7b1 to
b78cc0e
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 11, 2024
…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
JasonKChow
force-pushed
the
export-D67059839
branch
from
b78cc0e to
0bc0a76
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 11, 2024
…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
JasonKChow
force-pushed
the
export-D67059839
branch
from
0bc0a76 to
dec38c9
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…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
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…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. Reviewed By: crasanders Differential Revision: D67059839
JasonKChow
force-pushed
the
export-D67059839
branch
from
dec38c9 to
97137a6
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…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
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…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. Reviewed By: crasanders Differential Revision: D67059839
JasonKChow
force-pushed
the
export-D67059839
branch
from
97137a6 to
ebd7970
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…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
JasonKChow
force-pushed
the
export-D67059839
branch
from
ebd7970 to
3de0938
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 12, 2024
…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. Reviewed By: crasanders Differential Revision: D67059839
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 13, 2024
…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 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
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 14, 2024
…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
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 14, 2024
…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. Reviewed By: crasanders Differential Revision: D67059839
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
force-pushed
the
export-D67059839
branch
from
3de0938 to
aec7f9e
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 14, 2024
…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. Reviewed By: crasanders Differential Revision: D67059839
JasonKChow
force-pushed
the
export-D67059839
branch
from
aec7f9e to
68a6d32
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 16, 2024
…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
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 16, 2024
…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. Reviewed By: crasanders Differential Revision: D67059839
JasonKChow
force-pushed
the
export-D67059839
branch
from
68a6d32 to
91290f6
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
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 16, 2024
…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. Reviewed By: crasanders Differential Revision: D67059839
JasonKChow
force-pushed
the
export-D67059839
branch
from
91290f6 to
96ad5b1
Compare
JasonKChow
added a commit
to JasonKChow/aepsych
that referenced
this pull request
Dec 16, 2024
…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
…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. Reviewed By: crasanders Differential Revision: D67059839
|
This pull request was exported from Phabricator. Differential Revision: D67059839 |
JasonKChow
force-pushed
the
export-D67059839
branch
from
96ad5b1 to
4a333d8
Compare
|
This pull request has been merged in 02b24e7. |
JasonKChow
added a commit
that referenced
this pull request
Dec 18, 2024
…#478) Summary: Pull Request resolved: #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. Reviewed By: crasanders Differential Revision: D67059839 fbshipit-source-id: 868eb08de478eb3cb76c7b4a1b74a062ea64be24
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:
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