-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: Add percentile function to tensorlib #817
Conversation
Codecov Report
@@ Coverage Diff @@
## master #817 +/- ##
=======================================
Coverage 98.05% 98.05%
=======================================
Files 64 64
Lines 4212 4220 +8
Branches 587 587
=======================================
+ Hits 4130 4138 +8
Misses 49 49
Partials 33 33
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Note to self: Should give more information in docstrings of interpolation methods and note what is the default interpolation method ("linear"). |
12999fa
to
6d9469c
Compare
e79d986
to
75f591f
Compare
75f591f
to
f7fd797
Compare
f7fd797
to
2faeb7e
Compare
2faeb7e
to
3ff8978
Compare
3ff8978
to
6e4971e
Compare
7a6c3ff
to
3002b3a
Compare
3002b3a
to
511363e
Compare
src/pyhf/tensor/pytorch_backend.py
Outdated
|
||
""" | ||
# Interpolation options not yet supported | ||
# c.f. https://github.com/pytorch/pytorch/issues/38349 |
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.
should be out with pytorch/pytorch#48523 being done?
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.
Nope -- need 1.7.2 (https://github.com/pytorch/pytorch/releases/tag/v1.7.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.
Yeah, sadly. I just keep rebasing these PRs so that they don't fall too far behind and make it harder later on.
f9a52a4
to
2bddd5a
Compare
@pytest.mark.skip_pytorch | ||
@pytest.mark.skip_pytorch64 | ||
@pytest.mark.skip_jax |
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.
Skipping PyTorch as interpolation kwargs aren't implemented yet as of torch
v1.10.0
. c.f. #815 (comment)
Skipping JAX so as to have separate test below given floating point issues with "linear"
option.
assert tb.tolist(tb.percentile(a, 50, interpolation="higher")) == 4.0 | ||
|
||
|
||
@pytest.mark.only_jax |
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.
Hopefully these tests are tolerable at the moment, though obviously not ideal.
RE: the deviations seen for JAX compared to all other backends, I opened up jax-ml/jax#8513. |
Issue #1693 exists to monitor the PyTorch and JAX issues, so this can PR can move forward. |
Description
Partially addresses Issue Add a percentile function to tensorlib #815Add a
percentile
function to the tensor backends that adheres to NumPy'snumpy.percentile
API.To get quotation marks to look okay for the interpolation method section of the docstrings I used\\
to escape them given this Stack Overflow question. There may be a better way.Docs build at: https://pyhf--817.org.readthedocs.build/en/817/api.html#backends
Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: