-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix batch size #354
Fix batch size #354
Conversation
# update the training data to the current train_x and train_y, to avoid "You must train on the | ||
# training data!" | ||
self._gp.set_train_data(train_x, train_y.squeeze(dim=-1), strict=False) | ||
# TODO: consider using get_fantasy_model() instead if possible, when using ExactGP? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a sense for the effect on performance of not using get_fantasy_model()
? Is it more of a nice to have or is it possible batching could slow down the fitting as it is currently implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Batching is still a good deal faster than running on the whole dataset - on my machine, the unbatched test_base_usage integration test takes 2.87s for a 500-training-point dataset, but the test that does the same thing with batch_size=100
takes only 0.48s.
Strangely, setting batch_size=500
makes the test take only 1.85s, and setting batch_size=499
makes the test fail as 55% of outputs are outside the confidence interval! This should be investigated, but might have to be its own separate issue.
I don't know enough about gpytorch to comment on whether or by how much the performance might be improved by get_fantasy_model()
, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, that definitely sounds like it would need its own issue. I would be happy to merge after that has been set up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question. Also, is there anything that can be said about #266 given the changes here?
PR Type
Description
Closes #265. In batch mode, the GP's internal training data has to be reset at every training iteration to avoid it complaining that "You must train on the training inputs!".
How Has This Been Tested?
The base usage unit test has been refactored to also test batch mode.
Does this PR introduce a breaking change?
No.
Screenshots
N/A
Checklist before requesting a review