Add transition criteria to GenerationNode #1887
Closed
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
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D49509997 |
mgarrard
pushed a commit
to mgarrard/Ax
that referenced
this pull request
Oct 3, 2023
Summary: In this diff we do a few things: (1) Create a TransitionCriterion class: - this class will subsume the CompletionCriterion class and will be a bit more flexible - it has the same child classes + maximumtrialsinstatus subclass, we may add more subclasses or fields later as we further test (2) Create an list of transitioncriterion from the generationstep class: - minimum_trials_observed can be taken care of by the more flexible MinimumTrialsInStatus class - num_trials and enforce_num_trials can be taken care of by the more flexible MaximumTrialsInStatus class (3) adds a doc string to GenNode class - tangential but easy (4) updates the type of completion_criteria of GenerationStep from CompletionCriterion to TransitionCriterion (5) clean up compeletion_criterion class bc we don't need it anymore In following diffs we will: (1) add transition criterion to the repr string + some of the other fields that havent made it yet (2) begin moving the functions related to completing the step up to node and leveraging the transition criterion for checks instead of indexes -- this is where we may need to add additional fields to transitioncriterion (3) add doc strings to everywhere in teh GenNode class (4) add additional unit tests to MaxTrials to bring coverage to 100% (5) skip max trial criterion addition if numtrials == -1 Reviewed By: lena-kashtelyan Differential Revision: D49509997
mgarrard
force-pushed
the
export-D49509997
branch
from
9aa4a29 to
2246302
Compare
|
This pull request was exported from Phabricator. Differential Revision: D49509997 |
crasanders
pushed a commit
to crasanders/aepsych
that referenced
this pull request
Oct 3, 2023
Summary: X-link: facebook/Ax#1887 In this diff we do a few things: (1) Create a TransitionCriterion class: - this class will subsume the CompletionCriterion class and will be a bit more flexible - it has the same child classes + maximumtrialsinstatus subclass, we may add more subclasses or fields later as we further test (2) Create an list of transitioncriterion from the generationstep class: - minimum_trials_observed can be taken care of by the more flexible MinimumTrialsInStatus class - num_trials and enforce_num_trials can be taken care of by the more flexible MaximumTrialsInStatus class (3) adds a doc string to GenNode class - tangential but easy (4) updates the type of completion_criteria of GenerationStep from CompletionCriterion to TransitionCriterion (5) clean up compeletion_criterion class bc we don't need it anymore In following diffs we will: (1) add transition criterion to the repr string + some of the other fields that havent made it yet (2) begin moving the functions related to completing the step up to node and leveraging the transition criterion for checks instead of indexes -- this is where we may need to add additional fields to transitioncriterion (3) add doc strings to everywhere in teh GenNode class (4) add additional unit tests to MaxTrials to bring coverage to 100% (5) skip max trial criterion addition if numtrials == -1 Reviewed By: lena-kashtelyan Differential Revision: D49509997
mgarrard
pushed a commit
to mgarrard/Ax
that referenced
this pull request
Oct 3, 2023
Summary: X-link: facebookresearch/aepsych#320 In this diff we do a few things: (1) Create a TransitionCriterion class: - this class will subsume the CompletionCriterion class and will be a bit more flexible - it has the same child classes + maximumtrialsinstatus subclass, we may add more subclasses or fields later as we further test (2) Create an list of transitioncriterion from the generationstep class: - minimum_trials_observed can be taken care of by the more flexible MinimumTrialsInStatus class - num_trials and enforce_num_trials can be taken care of by the more flexible MaximumTrialsInStatus class (3) adds a doc string to GenNode class - tangential but easy (4) updates the type of completion_criteria of GenerationStep from CompletionCriterion to TransitionCriterion In following diffs we will: (1) add transition criterion to the repr string + some of the other fields that havent made it yet (2) begin moving the functions related to completing the step up to node and leveraging the transition criterion for checks instead of indexes -- this is where we may need to add additional fields to transitioncriterion (3) add doc strings to everywhere in teh GenNode class (4) add additional unit tests to MaxTrials to bring coverage to 100% (5) skip max trial criterion addition if numtrials == -1 (6) clean up compeletion_criterion class once new ax release can be pinned to aepsych version Reviewed By: lena-kashtelyan Differential Revision: D49509997
mgarrard
force-pushed
the
export-D49509997
branch
from
2246302 to
ea45292
Compare
|
This pull request was exported from Phabricator. Differential Revision: D49509997 |
mgarrard
pushed a commit
to mgarrard/Ax
that referenced
this pull request
Oct 4, 2023
Summary: X-link: facebookresearch/aepsych#320 In this diff we do a few things: (1) Create a TransitionCriterion class: - this class will subsume the CompletionCriterion class and will be a bit more flexible - it has the same child classes + maximumtrialsinstatus subclass, we may add more subclasses or fields later as we further test (2) Create an list of transitioncriterion from the generationstep class: - minimum_trials_observed can be taken care of by the more flexible MinimumTrialsInStatus class - num_trials and enforce_num_trials can be taken care of by the more flexible MaximumTrialsInStatus class (3) adds a doc string to GenNode class - tangential but easy (4) updates the type of completion_criteria of GenerationStep from CompletionCriterion to TransitionCriterion In following diffs we will: (1) add transition criterion to the repr string + some of the other fields that havent made it yet (2) begin moving the functions related to completing the step up to node and leveraging the transition criterion for checks instead of indexes -- this is where we may need to add additional fields to transitioncriterion (3) add doc strings to everywhere in teh GenNode class (4) add additional unit tests to MaxTrials to bring coverage to 100% (5) skip max trial criterion addition if numtrials == -1 (6) clean up compeletion_criterion class once new ax release can be pinned to aepsych version Reviewed By: lena-kashtelyan Differential Revision: D49509997
mgarrard
force-pushed
the
export-D49509997
branch
from
ea45292 to
ef76384
Compare
|
This pull request was exported from Phabricator. Differential Revision: D49509997 |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1887 +/- ##
=======================================
Coverage 94.58% 94.59%
=======================================
Files 458 460 +2
Lines 43522 43610 +88
=======================================
+ Hits 41166 41253 +87
- Misses 2356 2357 +1
☔ View full report in Codecov by Sentry. |
mgarrard
pushed a commit
to mgarrard/Ax
that referenced
this pull request
Oct 4, 2023
Summary: X-link: facebookresearch/aepsych#320 In this diff we do a few things: (1) Create a TransitionCriterion class: - this class will subsume the CompletionCriterion class and will be a bit more flexible - it has the same child classes + maximumtrialsinstatus subclass, we may add more subclasses or fields later as we further test (2) Create an list of transitioncriterion from the generationstep class: - minimum_trials_observed can be taken care of by the more flexible MinimumTrialsInStatus class - num_trials and enforce_num_trials can be taken care of by the more flexible MaximumTrialsInStatus class (3) adds a doc string to GenNode class - tangential but easy (4) updates the type of completion_criteria of GenerationStep from CompletionCriterion to TransitionCriterion In following diffs we will: (1) add transition criterion to the repr string + some of the other fields that havent made it yet (2) begin moving the functions related to completing the step up to node and leveraging the transition criterion for checks instead of indexes -- this is where we may need to add additional fields to transitioncriterion (3) add doc strings to everywhere in teh GenNode class (4) add additional unit tests to MaxTrials to bring coverage to 100% (5) skip max trial criterion addition if numtrials == -1 (6) clean up compeletion_criterion class once new ax release can be pinned to aepsych version Reviewed By: lena-kashtelyan Differential Revision: D49509997
mgarrard
force-pushed
the
export-D49509997
branch
from
ef76384 to
6fea95d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D49509997 |
Summary: X-link: facebookresearch/aepsych#320 In this diff we do a few things: (1) Create a TransitionCriterion class: - this class will subsume the CompletionCriterion class and will be a bit more flexible - it has the same child classes + maximumtrialsinstatus subclass, we may add more subclasses or fields later as we further test (2) Create an list of transitioncriterion from the generationstep class: - minimum_trials_observed can be taken care of by the more flexible MinimumTrialsInStatus class - num_trials and enforce_num_trials can be taken care of by the more flexible MaximumTrialsInStatus class (3) adds a doc string to GenNode class - tangential but easy (4) updates the type of completion_criteria of GenerationStep from CompletionCriterion to TransitionCriterion In following diffs we will: (1) add transition criterion to the repr string + some of the other fields that havent made it yet (2) begin moving the functions related to completing the step up to node and leveraging the transition criterion for checks instead of indexes -- this is where we may need to add additional fields to transitioncriterion (3) add doc strings to everywhere in teh GenNode class (4) add additional unit tests to MaxTrials to bring coverage to 100% (5) skip max trial criterion addition if numtrials == -1 (6) clean up compeletion_criterion class once new ax release can be pinned to aepsych version Reviewed By: lena-kashtelyan Differential Revision: D49509997
mgarrard
force-pushed
the
export-D49509997
branch
from
6fea95d to
e0a2ff3
Compare
|
This pull request was exported from Phabricator. Differential Revision: D49509997 |
|
This pull request has been merged in 031fa35. |
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:
In this diff we do a few things:
(1) Create a TransitionCriterion class:
(2) Create an list of transitioncriterion from the generationstep class:
(3) adds a doc string to GenNode class - tangential but easy
(4) updates the type of completion_criteria of GenerationStep from CompletionCriterion to TransitionCriterion
(5) clean up compeletion_criterion class bc we don't need it anymore
In following diffs we will:
(1) add transition criterion to the repr string + some of the other fields that havent made it yet
(2) begin moving the functions related to completing the step up to node and leveraging the transition criterion for checks instead of indexes -- this is where we may need to add additional fields to transitioncriterion
(3) add doc strings to everywhere in teh GenNode class
(4) add additional unit tests to MaxTrials to bring coverage to 100%
(5) skip max trial criterion addition if numtrials == -1
Reviewed By: lena-kashtelyan
Differential Revision: D49509997