-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add numpy like percentile and quantile #942
base: main
Are you sure you want to change the base?
Add numpy like percentile and quantile #942
Conversation
658e615
to
cc8d4d9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #942 +/- ##
==========================================
+ Coverage 81.38% 81.61% +0.22%
==========================================
Files 172 178 +6
Lines 46868 47285 +417
Branches 11423 11497 +74
==========================================
+ Hits 38145 38592 +447
+ Misses 6542 6504 -38
- Partials 2181 2189 +8
|
cc8d4d9
to
77d8604
Compare
pytensor/tensor/math.py
Outdated
|
||
percentiles = [100.0 * x for x in q] | ||
|
||
return percentile(input, percentiles, axis) |
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.
This is a but irrelevant, but does it make sense to define quantile first and then percentile from quantile. So the other way around?
Quantile seems more fundamental
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.
Yes makes sense. I have pushed this.
14967fe
to
c3bda8a
Compare
pytensor/tensor/math.py
Outdated
for quantile in q: | ||
if quantile < 0 or quantile > 1: | ||
raise ValueError("Quantiles must be in the range [0, 1]") | ||
|
||
result = [] | ||
for quantile in q: | ||
k = (quantile) * (input_shape - 1) | ||
k_floor = floor(k).astype("int64") | ||
k_ceil = ceil(k).astype("int64") | ||
|
||
slices1[-1] = slice(k_floor, k_floor + 1) | ||
slices2[-1] = slice(k_ceil, k_ceil + 1) | ||
val1 = sorted_input[tuple(slices1)] | ||
val2 = sorted_input[tuple(slices2)] | ||
|
||
d = k - k_floor | ||
quantile_val = val1 + d * (val2 - val1) | ||
|
||
result.append(quantile_val.squeeze(axis=-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.
This won't work with symbolic q. Can we have a vectorized implementation?
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.
Hi. Sorry this completely skipped me. I'll get back to it and see how to vectorise this and also look into the failing tests.
This code could look a lot like the median one, with the exception that the k in We should be able to re-implement the median as |
c3bda8a
to
dcdeef7
Compare
Hi @ricardoV94 I have added some changes. However I need some help in some parts. For "ndim, axis, q" equal to (3, (1, 2), [0.1, 0.9]), the tests fail due to some shapes issues:
Also, I am confused on how to introduce symbolic checks to ensure the all of the q is between 0 to 1 in case of quantile and 0 to 100 in case of percentile. |
We can be a bit permissive and consider it undefined behavior if it makes our code simpler. Otherwise there's a |
I'll try to take a look when I get some free time |
Description
This pr aims to add support for
numpy
like percentile and quantile functions.Related Issue
Checklist
Type of change