Skip to content
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

Document how to implement custom models #2474

Closed
wants to merge 14 commits into from
Closed

Conversation

jakee417
Copy link
Contributor

Motivation

Issue #2306

Have you read the Contributing Guidelines on pull requests?

Yes. Added a tutorial which can be used for smoke tests.

Test Plan

Probabilistic linear regression, bayesian linear regression, and ensemble linear regression all yield optimization results close to (0, 0) which is groundtruth answer.

Random Forest doesn't seem to achieve groundtruth answer, likely due to its inability to incorporate gradient information into the optimization of the acquisition function.

Related PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 20, 2024
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.98%. Comparing base (dd6448b) to head (0087151).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2474   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files         191      191           
  Lines       16856    16864    +8     
=======================================
+ Hits        16854    16862    +8     
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@saitcakmak saitcakmak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great tutorial, thank you very much for the contribution! I just have minor point about the sampler registration. Once it is ready, you can import the PR to fbcode. Landing it in fbcode will sync the changes to GitHub and close the PR.

tutorials/custom_model.ipynb Outdated Show resolved Hide resolved
tutorials/custom_model.ipynb Outdated Show resolved Hide resolved
@saitcakmak saitcakmak linked an issue Aug 21, 2024 that may be closed by this pull request
Copy link
Contributor

@Balandat Balandat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome tutorial, thanks a lot for the contribution!

Some additional linear algebraic comments:

        # Inverse of the gram matrix.
        self.V = torch.linalg.inv(x.T @ x)

It's generally inadvisable to compute the inverse of a matrix. Rather than doing that, you'd typically compute a matrix decomposition of x.T @ x and use that for solves down the line using forward-backward substitutions. Could you modify the code to that end? Happy to provide more specifics / code changes if that would be useful.

Also, some of this can get hairy in 32bit precision if the gram matrix is ill-conditioned. In general we recommend folks use botorch with 64bit precision (except when running on larger data sets on a GPU where perf really counts). Could you modify the tutorial to use torch.float64? Either by choosing the dtype explicitly or by setting the default torch dtype at the beginning of the tutorial.

@jakee417
Copy link
Contributor Author

jakee417 commented Aug 21, 2024

This is an awesome tutorial, thanks a lot for the contribution!

Some additional linear algebraic comments:

        # Inverse of the gram matrix.
        self.V = torch.linalg.inv(x.T @ x)

It's generally inadvisable to compute the inverse of a matrix. Rather than doing that, you'd typically compute a matrix decomposition of x.T @ x and use that for solves down the line using forward-backward substitutions. Could you modify the code to that end? Happy to provide more specifics / code changes if that would be useful.

Also, some of this can get hairy in 32bit precision if the gram matrix is ill-conditioned. In general we recommend folks use botorch with 64bit precision (except when running on larger data sets on a GPU where perf really counts). Could you modify the tutorial to use torch.float64? Either by choosing the dtype explicitly or by setting the default torch dtype at the beginning of the tutorial.

torch.Double seems to improve the results, I see why this is recommended now. Switched the torch.linalg.inv out for torch.linalg.cholesky followed by torch.cholesky_inverse. I am not sure if torch.cholesky_inverse does forwards-backwards under the hood, but it gives the same result as torch.linalg.inv.

@Balandat
Copy link
Contributor

Balandat commented Aug 21, 2024

torch.Double seems to improve the results, I see why this is recommended now.

Great

Switched the torch.linalg.inv out for torch.linalg.cholesky followed by torch.cholesky_inverse. I am not sure if torch.cholesky_inverse does forwards-backwards under the hood, but it gives the same result as torch.linalg.inv.

torch.cholesky_inverse still computes the inverse explicitly. It's better than torch.linalg.inv since you're using the knowledge that the matrix is pd, but instead of computing the inverse explicitly and then doing matmuls you should use torch.cholesky_solve (which does forward-backward substitution). So if you have A x = b you want to do x = torch.cholesky_solve(b, torch.linalg.cholesky(A)) instead of x = torch.cholesky_inverse(torch.linalg.cholesky(A)) @ x. You can assign self.L = torch.linalg.cholesky(A) instead of self.V = torch.cholesky_inverse(L) and use that both to compute the Least squares estimate as well as in the posterior call (swapping out self.V @ X.squeeze().T for the respective cholesky_solve() call).

@jakee417
Copy link
Contributor Author

jakee417 commented Aug 21, 2024

okay, now I got it. Replaced the V @ x with torch.cholesky_solve(b, L) in the appropriate places.

@facebook-github-bot
Copy link
Contributor

@jakee417 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@@ -153,4 +157,4 @@
"title": "Composite Bayesian Optimization with Multi-Task Gaussian Processes"
}
]
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened here, a whitespace change?

Copy link
Member

@esantorella esantorella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this is a great tutorial and I may refer to it myself in the future. I left a couple minor suggestions, but this looks pretty good to me as-is.

@facebook-github-bot
Copy link
Contributor

@jakee417 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@jakee417 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@jakee417 merged this pull request in 81f2a88.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
5 participants