-
Notifications
You must be signed in to change notification settings - Fork 356
Clean up center generation node implementation #4734
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
Open
mgarrard
wants to merge
2
commits into
facebook:main
Choose a base branch
from
mgarrard:export-D90040731
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+326
−287
Conversation
This file contains hidden or 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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4734 +/- ##
==========================================
- Coverage 96.72% 96.72% -0.01%
==========================================
Files 582 582
Lines 60699 60723 +24
==========================================
+ Hits 58713 58736 +23
- Misses 1986 1987 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Jan 5, 2026
Summary: This diff cleans up center generation node logic to hopefully make it a bit more eloquent. It does the following: * Removes AutoTransitionAfterGenOrExhaustion as it is duplicative of AutoTransitionAfterGen, it just has a different defalt * Updates base ExternalGenerationNode to have a gen override which allows for `should_skip` to be set by a helper method, and called prior to super.gen() -- this allows for skiping to for special cases like center_generation_node, without updating the core logic in generation strategy (which would have required checking after fitting, and that would have determential consequences on generation time) * stores center parms in update_generator_state - prevents any recomputations that are unnecessary, center will remain the same for one given experiment state at time of gen * removes custom gen override in center_generation_node Notably, it would be only in a misconfigured state that we would use sobol fallback now, this is consistent with David's initial changes, see D87797277 for explaination of that chage -- we could even remove sobol fallback from the node if we want Differential Revision: D90040731 Privacy Context Container: L1307644
46ed9c3 to
98fe0d6
Compare
Summary: This diff implements Chebyshev center if naive centering fails due to violation of parameter constraints. Differential Revision: D85712042
Summary: This diff cleans up center generation node logic to hopefully make it a bit more eloquent. It does the following: * Removes AutoTransitionAfterGenOrExhaustion as it is duplicative of AutoTransitionAfterGen, it just has a different defalt * Updates base ExternalGenerationNode to have a gen override which allows for `should_skip` to be set by a helper method, and called prior to super.gen() -- this allows for skiping to for special cases like center_generation_node, without updating the core logic in generation strategy (which would have required checking after fitting, and that would have determential consequences on generation time) * stores center parms in update_generator_state - prevents any recomputations that are unnecessary, center will remain the same for one given experiment state at time of gen * removes custom gen override in center_generation_node Notably, it would be only in a misconfigured state that we would use sobol fallback now, this is consistent with David's initial changes, see D87797277 for explaination of that chage -- we could even remove sobol fallback from the node if we want Differential Revision: D90040731 Privacy Context Container: L1307644
98fe0d6 to
0d5e25a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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:
This diff cleans up center generation node logic to hopefully make it a bit more eloquent. It does the following:
should_skipto be set by a helper method, and called prior to super.gen() -- this allows for skiping to for special cases like center_generation_node, without updating the core logic in generation strategy (which would have required checking after fitting, and that would have determential consequences on generation time)Notably, it would be only in a misconfigured state that we would use sobol fallback now, this is consistent with David's initial changes, see D87797277 for explaination of that chage -- we could even remove sobol fallback from the node if we want
Differential Revision:
D90040731
Privacy Context Container: L1307644