Fixes inverse cost-weighted utility behaviour for AF values <=0 #2297
Conversation
Instead of clamping negative AF values to 0 or dividing it by the cost, this changes the behaviour to multiply AF values by the cost if they are <=0. One unit test asserting that the value of the AF has been clamped was also removed.
facebook-github-bot
added
the
CLA Signed
|
@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -304,26 +304,6 @@ def test_evaluate_q_hvkg(self): | |||
| self.assertTrue( | |||
| torch.equal(qHVKG.extract_candidates(X), X[..., : -n_f * num_pareto, :]) | |||
| ) | |||
There was a problem hiding this comment.
For cost-aware optimization, could you add a unit test demonstrating that this chooses the right candidate in cases where the logic has changed (negative deltas)?
And for HVKG, is there some way of demonstrating that it's still giving the "right" answer with the clamping removed? It might be good to keep this test in place but just change the assert at the end. I'm not very familiar with HVKG, so this may not turn out to be a good suggestion.
There was a problem hiding this comment.
Hi @esantorella
Thanks for the feedback!
I'll add a unit test for demonstrating that it chooses the right candidate tomorrow.
I'm unfortunately not familiar with HVKG either... Any thoughts on this? @sdaulton @Balandat
There was a problem hiding this comment.
One way would be to run the MFKG tutorial: https://github.com/pytorch/botorch/blob/main/tutorials/Multi_objective_multi_fidelity_BO.ipynb - not sure if this does run into issues with negative improvements, but it's an easy first-order test (you can make a local edit in the code to check whether that new functionality is triggered)
There was a problem hiding this comment.
Hi @esantorella and @Balandat
I've added tests that demonstrate the correct functionality in the test_cost_aware.py file, as I think it makes more sense to have it there. (The alternative being to add tests for it in the tests classes associated with MFKG and HVKG)
I've also run the MFKG tutorial you mentioned @Balandat. Below are some of the printouts for the (negative-only) AF values before and after scaling by the cost:
-0.07815102929370721 -> -2.9963066532177565
-0.07778381446031779 -> -7.4577009200049895
-0.08422953299033326 -> -5.449204194716502
...
-0.10146312396850868 -> -0.10146312396850868
-0.10146311575673453 -> -0.10146311575673453
-0.10146311575673453 -> -0.10146311575673453
The change also seemed to improve the HVKG results from a regret in the range [-1.55, -1.6] to [-1.6, -1.65]. (The runs in the tutorial are seeded)
Before:
After:
Should I rerun and add the tutorials that use the InverseCostWeightedUtility to this PR as well?
Thanks!
There was a problem hiding this comment.
Oh yeah, it would be great to re-run those tutorials if the results change. Good catch, thanks!
Adds tests to verify that negative AF values are multiplied by the cost, and not divided as previously. See discussion pytorch#2194.
| # test that ensures candidates of lower cost are preferred when they | ||
| # have equal, negative delta values |
| @@ -304,26 +304,6 @@ def test_evaluate_q_hvkg(self): | |||
| self.assertTrue( | |||
| torch.equal(qHVKG.extract_candidates(X), X[..., : -n_f * num_pareto, :]) | |||
| ) | |||
There was a problem hiding this comment.
Oh yeah, it would be great to re-run those tutorials if the results change. Good catch, thanks!
|
@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| return deltas / cost | ||
| # this operation involves broadcasting the cost across fantasies. | ||
| # We multiply by the cost if the deltas are <= 0, see discussion #2914 | ||
| return torch.where(deltas > 0, deltas / cost, deltas * cost) |
There was a problem hiding this comment.
Thanks for putting this up! Could you update the doc-string of forward here, as well as the math in the class description? Otherwise looks good to me and ready to merge.
There was a problem hiding this comment.
Hi @SebastianAment and @esantorella
I've rerun the MOMFBO notebook and updated the relevant doc-strings. The other 3 notebooks that use the InverseCostWeightedUtility weren't significantly affected by the change; or rather they don't use manually set seeds as with the MOMFBO notebook, so I could not see a significant difference in the results.
Thanks!
Updates the doc-strings for the InverseCostWeightedUtility class to reflect the changes to how negative AF values are handled.
The change in how the InverseCostWeightedUtility class handles negative costs changed the results of this tutorial, and thus needed to be rerun.
|
@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@esantorella merged this pull request in f6b7530. |
Instead of clamping negative AF values to 0.0 or dividing it by the cost, this changes the behaviour to multiply AF values by the cost if they are <=0.0. One test asserting that the value of the AF has been clamped was removed, as it is no longer applicable.
Motivation
This PR addresses the issue discussed in #2194, where dividing negative KG AF values by cost results in more expensive points of equal or lower value being regarded as having higher AF values.
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
Only minor code changes were necessary, where the unit tests already account for the changes. Furthermore, I have been using this version of the
InverseCostWeightedUtilityin my own experiments.Related PRs
N/A