fix quantile fn for transformed distribution #544
Conversation
Codecov Report
@@ Coverage Diff @@
## master #544 +/- ##
==========================================
- Coverage 83.16% 83.15% -0.01%
==========================================
Files 177 177
Lines 9864 9869 +5
==========================================
+ Hits 8203 8207 +4
- Misses 1661 1662 +1
|
| q_pos = self.base_distribution.quantile(level) | ||
| q_neg = self.base_distribution.quantile(1.0 - level) | ||
| cond = F.broadcast_greater(sign, sign.zeros_like()) | ||
| cond = F.broadcast_add( | ||
| cond.expand_dims(axis=1), q_pos.zeros_like() | ||
| ) | ||
| q = F.where(cond, q_pos, q_neg) |
There was a problem hiding this comment.
This makes two calls to .quantile instead of one. Wouldn't something like
cond = F.broadcast_greater(sign, sign.zeros_like())
signed_level = F.where(cond, level, 1.0 - level)
q = self.base_distribution.quantile(signed_level)be more efficient?
There was a problem hiding this comment.
This is what we had before. The problem is that in the current API for .quantile level is supposed to be a 1d tensor of level values, i.e., it doesn't know about the shape of the distribution. The previous code blew up the level to a larger tensor mixing in the shape of sign. It did work (not entirely sure how), but it resulted in very large tensors during the quantile computation that don't fit in GPU memory for e.g. Binned distribution when the distribution dimension was large. Maybe we can rethink the api for quantile to not only support 1d level values, but also level values per distribution dimension.
There was a problem hiding this comment.
Makes sense, thanks for clarifying. If that’s the contract that quantile assumes (scalar or 1D tensor) then I agree that your solution is probably the best.
In a separate story, we may want to start shape assertions in order to avoid these weird situations where things accidentally work :-) (maybe because of some strange broadcasting coincidence)
|
Do we have a use case for these bijections with negative Jacobian determinant (i.e. monotonically decreasing)? |
|
Good point. Maybe we should just remove the sign stuff and document clearly that we expect positive determinant. |
Description of changes:
Fix quantile function for transformed distribution and minor other improvements.
This reduces memory consumption when calculating quantiles for transformed distribution.
Also minor improvements for forecast methods.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.