Skip to content

Point to example for constraints def in optimize_acqf docstring #1900

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

Closed
wants to merge 1 commit into from

Conversation

Balandat
Copy link
Contributor

Summary: In response to #1899

Differential Revision: D46900427

Summary: In response to pytorch#1899

Differential Revision: D46900427

fbshipit-source-id: 5c903e9aa876095554ecc5167bdcd89ba9d082fb
@facebook-github-bot facebook-github-bot added CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported labels Jun 21, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46900427

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #1900 (dfcfc80) into main (e5bf538) will not change coverage.
The diff coverage is n/a.

❗ Current head dfcfc80 differs from pull request most recent head 84e06db. Consider uploading reports for the commit 84e06db to get more accurate results

@@            Coverage Diff            @@
##              main     #1900   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          173       173           
  Lines        15194     15194           
=========================================
  Hits         15194     15194           
Impacted Files Coverage Δ
botorch/optim/optimize.py 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -446,7 +446,8 @@ def optimize_acqf(
inequality_constraints: A list of tuples (indices, coefficients, rhs),
with each tuple encoding an inequality constraint of the form
`\sum_i (X[indices[i]] * coefficients[i]) >= rhs`. `indices` and
`coefficients` should be torch tensors. When q=1, or when
`coefficients` should be torch tensors. See the docstring of
`make_scipy_linear_constraints` for an example. When q=1, or when
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely an improvement but what is make_scipy_linear_constraints and where does one find it? It is not imported in this file, so the user would have to search the repo to find where it is defined.

Choose a reason for hiding this comment

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

google botorch make_scipy_linear_constraints. Also, look at this post I made when trying to understand how it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, this could definitely improved upon, but I wanted to get something out that's actionable (search/googleable) right away rather than putting this on a backlog list and then forget about it :)

Choose a reason for hiding this comment

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

Adding the

see the docstring of make_scipy_linear_constraints for an example

allows for googling

I just searched for it and the 3rd option was the charm, giving me this link, where you can find, with a bit more search, this part of the page
image

I can see it's not the best option, but at least it allows the user to understand what's going on behind the scenes.

Choose a reason for hiding this comment

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

You could also link to this question I made in stackoverflow in the docs. IT's just my rambling and understanding of how the process is working, but could be useful

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 46d4c51.

@Balandat Balandat deleted the export-D46900427 branch March 5, 2024 14:34
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. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants