-
Notifications
You must be signed in to change notification settings - Fork 750
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
include quantile function in distribution #314
Conversation
Job PR-314/1 is complete. |
2f1f193
to
c91bd79
Compare
Job PR-314/2 is complete. |
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
===========================================
+ Coverage 40.45% 80.54% +40.08%
===========================================
Files 142 142
Lines 7966 8024 +58
===========================================
+ Hits 3223 6463 +3240
+ Misses 4743 1561 -3182
|
@@ -223,6 +230,12 @@ def cdf(self, x: Tensor) -> Tensor: | |||
""" | |||
raise NotImplementedError() | |||
|
|||
def quantile(self, level: Tensor) -> Tensor: | |||
r""" | |||
Returns the quantile corresponding to the passed level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to document level
here as well as the return value (shape and so on)
def quantile(self, level: Tensor) -> Tensor: | ||
F = self.F | ||
# we consider level to be an independent axis and so expand it | ||
# to shape (num_levels, 1, 1, ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, this expansion here is a bit counterintuitive if one reasons in terms of numpy’s broadcasting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah, I guess we should be looking at how mxnet does broadcasting :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we may need to iterate a bit on this. For now I assume the simplest case where level is a 1d array and the method returns a tensor with (num_levels, ...DISTRIBUTION SHAPE...)
. I documented this in the base class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, thanks
c91bd79
to
adcffaf
Compare
Job PR-314/3 is complete. |
Issue #, if available:
Description of changes:
Include a
quantile
function for distributions.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.