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

Fix output tensor dimension in degenerate optimize_acqf call #2743

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AdrianSosic
Copy link
Contributor

@AdrianSosic AdrianSosic commented Feb 12, 2025

Motivation

Fixes #2740

@sdaulton: This is a draft for fixing the dimensionality issue. While the PR technically solves the problem, think it only addresses the symptom and not the underlying root cause. For the latter, I'd need some guidance from your end.

Specifically, something seems fishy about the tensor dimensions promised by the acquisition function base class. The docstring states that the input is a (b) x q x d-dim tensor and the output is a (b)-dim. The way I understand this is that b is an optional batching dimension. But if you read the contract, it would imply:

  • If you pass a b x q x d-dim tensor, you get a b-dim tensor back.
  • If you pass a q x d-dim tensor, you get a dimensionless tensor (i.e. scalar value) back.

Now the problem is that the second part seems to be actually violated, which causes the the problem in #2740. Interestingly, when writing the test, I wanted to use a MockAcquisitionFunction() object for it like done in other tests. However, that object does seem to fulfill the contract, so I couldn't even reproduce the problem with the mock but had to reuse the code from my original minimal example.

Any thoughts how we should go about this, i.e. is this actually a more fundamental problem in the acquisition function code?

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

A corresponding test has been included.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 12, 2025
@facebook-github-bot
Copy link
Contributor

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

@sdaulton
Copy link
Contributor

Thanks for the PR! It looks like a test is failing. Would you mind fixing that please?

@AdrianSosic AdrianSosic force-pushed the fix/fixed_features_dimensionality branch from 68e3d1e to 3ced166 Compare February 19, 2025 08:58
@AdrianSosic
Copy link
Contributor Author

Thanks for the PR! It looks like a test is failing. Would you mind fixing that please?

Sorry, my bad, forgot to run the entire test suite. Should now be fixed – though there are 4 other tests failing also on the main branch?

Still, like I mentioned earlier: while the change fixes the particular problem reported, I don't think it properly addresses the root cause. So input would be welcome 🙃

Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.99%. Comparing base (9a7c517) to head (3ced166).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2743   +/-   ##
=======================================
  Coverage   99.99%   99.99%           
=======================================
  Files         203      203           
  Lines       18690    18692    +2     
=======================================
+ Hits        18689    18691    +2     
  Misses          1        1           

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

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
Development

Successfully merging this pull request may close these issues.

[Bug]: Inconsistent output dimensions
3 participants