Ax Trial: Bug fix for error message that references nonexistent function
#2304
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: D55439108 |
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Mar 28, 2024
…ction (facebook#2304) Summary: `BaseTrial._validate_can_attach_data` raises the following exception: f"Trial {self.index} has already been completed with data. To add more data to it (for example, for a different metric), use `Trial.update_trial_data()` or BatchTrial.update_batch_trial_data()." This is not right because there is no `BatchTrial.update_batch_trial_data`. Also, it's an anti-pattern for a superclass to reference its subclasses like this instead of delegating to them. This PR: - Makes the behavior in the base class make sense for clases that have no method like "update trial data" - Overrides that error message for `Trial` which does have such an update method Differential Revision: D55439108
esantorella
force-pushed
the
export-D55439108
branch
from
72f135c
to
7532c6f
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55439108 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2304 +/- ##
=======================================
Coverage 94.90% 94.91%
=======================================
Files 489 489
Lines 47690 47710 +20
=======================================
+ Hits 45262 45282 +20
Misses 2428 2428 ☔ View full report in Codecov by Sentry. |
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Mar 28, 2024
…ction (facebook#2304) Summary: `BaseTrial._validate_can_attach_data` raises the following exception: f"Trial {self.index} has already been completed with data. To add more data to it (for example, for a different metric), use `Trial.update_trial_data()` or BatchTrial.update_batch_trial_data()." This is not right because there is no `BatchTrial.update_batch_trial_data`. Also, it's an anti-pattern for a superclass to reference its subclasses like this instead of delegating to them. This PR: - Makes the behavior in the base class make sense for clases that have no method like "update trial data" - Overrides that error message for `Trial` which does have such an update method Differential Revision: D55439108
esantorella
force-pushed
the
export-D55439108
branch
from
7532c6f
to
2421578
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55439108 |
…ction (facebook#2304) Summary: `BaseTrial._validate_can_attach_data` raises the following exception: f"Trial {self.index} has already been completed with data. To add more data to it (for example, for a different metric), use `Trial.update_trial_data()` or BatchTrial.update_batch_trial_data()." This is not right because there is no `BatchTrial.update_batch_trial_data`. Also, it's an anti-pattern for a superclass to reference its subclasses like this instead of delegating to them. This PR: - Makes the behavior in the base class make sense for clases that have no method like "update trial data" - Overrides that error message for `Trial` which does have such an update method Reviewed By: bernardbeckerman, mpolson64 Differential Revision: D55439108
esantorella
force-pushed
the
export-D55439108
branch
from
2421578
to
c8aa2d6
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55439108 |
|
This pull request has been merged in 86a4c8d. |
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:
BaseTrial._validate_can_attach_dataraises the following exception: f"Trial {self.index} has already been completed with data. To add more data to it (for example, for a different metric), useTrial.update_trial_data()or BatchTrial.update_batch_trial_data()."This is not right because there is no
BatchTrial.update_batch_trial_data. Also, it's an anti-pattern for a superclass to reference its subclasses like this instead of delegating to them.This PR:
Trialwhich does have such an update methodDifferential Revision: D55439108