Minor updates to AxClient.fit_model #2580
Conversation
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D59778614 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2580 +/- ##
=======================================
Coverage 95.22% 95.22%
=======================================
Files 489 489
Lines 47647 47642 -5
=======================================
- Hits 45372 45369 -3
+ Misses 2275 2273 -2 ☔ View full report in Codecov by Sentry. |
| @@ -1197,9 +1198,10 @@ def get_model_predictions( | |||
| "argument to `get_model_predictions`." | |||
There was a problem hiding this comment.
A few lines above, it says that AxClient only supports one-arm trials. Is that saying that AxClient doesn't support BatchTrials? I didn't think that was the case. See e.g. here:
Line 1735 in 95881f4
There was a problem hiding this comment.
I don't know about AxClient more generally but this seems to be a limitation of the get_model_predictions method. I'll clarify this in the docstring.
| # can be performed without the need to call get_next_trial(), we update the | ||
| # model with all attached data. Note that this method keeps track of previously | ||
| # seen trials and will update the model if there is newly attached data. | ||
| self.generation_strategy._maybe_transition_to_next_node() |
There was a problem hiding this comment.
Why might we need to transition to the next node?
There was a problem hiding this comment.
This could go either way. With this call, we fit the model that would be used to generate the next trial. Without it, we would just re-train (with updated data) the model that was used to fit the last trial. I'll add a note about this to the docstring.
Summary: Pull Request resolved: facebook#2580 I came across `AxClient.fit_model` while working on something else. It appeared to be a method that was split off from `get_model_predictions`, with leftover error messages that did not fit well with it as a standalone non-protected method. This diff clears up the error messages and simplifies the definition of `AxClient.fit_model`. Differential Revision: D59778614
|
This pull request was exported from Phabricator. Differential Revision: D59778614 |
saitcakmak
force-pushed
the
export-D59778614
branch
from
004953b
to
b56f7b5
Compare
|
This pull request has been merged in 56c9b46. |
Summary:
I came across
AxClient.fit_modelwhile working on something else. It appeared to be a method that was split off fromget_model_predictions, with leftover error messages that did not fit well with it as a standalone non-protected method.This diff clears up the error messages and simplifies the definition of
AxClient.fit_model.Differential Revision: D59778614