Make get_infeasible_cost return a cost value for each outcome.
#1191
Conversation
Summary: Current implementation returns a single `M` value. This modifies it to return an `m`-dim tensor of infeasible cost values, each corresponding to one of the `m` outcomes. Differential Revision: D35847505 fbshipit-source-id: 6617a9285b61f57c9d90cbf59cfae3c9c56296c2
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D35847505 |
Codecov Report
@@ Coverage Diff @@
## main #1191 +/- ##
=======================================
Coverage 99.99% 99.99%
=======================================
Files 115 115
Lines 10134 10137 +3
=======================================
+ Hits 10133 10136 +3
Misses 1 1
Continue to review full report at Codecov.
|
| lb = objective(posterior.mean - 6 * posterior.variance.clamp_min(0).sqrt()) | ||
| if lb.ndim < posterior.mean.ndim: | ||
| lb = lb.unsqueeze(-1) | ||
| # Take outcome-wise min. Looping in to handle batched models. |
There was a problem hiding this comment.
So If I read this correctly, this computes the minimum across all batch dimensions? Is that what we want here? The previous implementation also did this so it's probably fine to get this in as is, but we may want to think about this some more if the batch dims correspond to different data and we want to extract them individually (this may be relevant for fully bayesian models - @dme65).
There was a problem hiding this comment.
Yeah, that is an interesting thought. I guess it would depend on how different we expect each model in the batch to be. Since they're all trained on the same set of inputs, I'd presume they'd be relatively similar, so one cost value should work for all. This would also affect the fantasized models, so maybe if we have two fantasy models, one on a large fantasy observation and the other on a small one, the cost value for one may be too conservative for the other? One could also argue that the 6 sigma we use here is too conservative to begin with. Anyways, I'm happy to revisit this later if we think it is worth exploring in more detail.
| @@ -320,7 +320,7 @@ def test_constrained_mc_objective(self): | |||
| obj=constrained_obj, | |||
| constraints=[feasible_con, infeasible_con], | |||
| samples=samples, | |||
| infeasible_cost=0.0, | |||
| infeasible_cost=torch.tensor([0.0], device=self.device, dtype=dtype), | |||
There was a problem hiding this comment.
you're accepting Union[Tensor, float] - should we test support both input types?
There was a problem hiding this comment.
There are something like 6 tests in there. I only changed two of them to use tensors, so both float and tensor are being tested.
Summary: X-link: facebook/Ax#1191 Pull Request resolved: #1439 This diff acts as follow-up to the recent model fitting refactor. The previous update focused on the high-level logic used to determine which fitting routines to use for which MLLs. This diff refactors the internal machinery used to evaluate forward-backward passes (producing losses and gradients, respectively) during optimization. The solution we have opted for is to abstract away the evaluation process by relying on closures. In most cases, these closures are automatically constructed by composing simpler, multiply-dispatched base functions. Reviewed By: Balandat Differential Revision: D39101211 fbshipit-source-id: c2058a387fd74058073cfe73c9404d2df2f9b55a
Summary: Current implementation returns a single
Mvalue. This modifies it to return anm-dim tensor of infeasible cost values, each corresponding to one of themoutcomes.Differential Revision: D35847505