Add loc argument to distribution output classes
#540
Conversation
Codecov Report
@@ Coverage Diff @@
## master #540 +/- ##
==========================================
- Coverage 83.18% 83.18% -0.01%
==========================================
Files 177 177
Lines 9877 9882 +5
==========================================
+ Hits 8216 8220 +4
- Misses 1661 1662 +1
|
There was a problem hiding this comment.
This PR reminds me that we actually use the wrong abstraction for the AffineTransformation transformed output. I think the right abstraction would be to have some kind of ConditionalBijection to model this (not for this PR, but we should think about this in the future).
| return self.distr_cls(*distr_args) | ||
| else: | ||
| distr = self.distr_cls(*distr_args) | ||
| return TransformedDistribution( | ||
| distr, [AffineTransformation(scale=scale)] | ||
| distr, [AffineTransformation(loc=loc, scale=scale)] |
There was a problem hiding this comment.
loc is actually never passed in any of our models, right? See DeepAR here for example.
There was a problem hiding this comment.
It's not indeed, and will default to None so long as it's not used
There was a problem hiding this comment.
To expand on this a bit: the reason for having loc alongside scale is to allow using a more general scaling, like min-max scaling or normalizing data (in the true sense of the word) instead of just dividing by some scaling factor. And if this is doable in a minimally invasive, 100% backwards compatible manner why not
@mbohlkeschneider yes, I think we could adjust a bit how distributions and transformations interact. Especially in the ubiquitous case of scaling, where we "manually" transform data before the model input layer, and use transformed distributions in the likelihood instead. |
Description of changes: This PR adds the
locargument to thedistributionmethod ofDistributionOutputclasses. Assertions are added in case the specific distributions don't support eitherlocorscale. A bug in theTransformedPiecewiseLineardistribution was also fixed: this went unnoticed since it only appears when the base distribution is composed with more than one affine transformation, which never happens when usingPiecewiseLinearOutput.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.