Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ENH: stats: add nonparametric one-sample quantile test and CI #12680
ENH: stats: add nonparametric one-sample quantile test and CI #12680
Changes from 15 commits
ad36fc4
896e431
70304eb
e3571b6
2c3a3d7
fb5f8a8
9e33d1e
e179d68
8483fc4
07abe2f
59bb0ee
2a5c286
ecff946
1b3542b
3332939
5c164ac
b9dab64
342278b
5db55d2
bcfe26c
765b1ce
3cb4f3b
3586c86
948810a
de88bd0
84c33e6
790cd76
59d37b5
c528102
ad996a5
c32fd1f
862701d
21842fe
786a89b
9ea76a6
5c34c14
222137e
acce928
6b9355c
74764b8
2e95192
7e75fe7
9a9fdab
ee9eae8
db934ff
9bf4fc3
89d09e0
616c59a
fa9599f
b020bff
daf468a
b29d329
0d072f7
ac56ad7
1661387
83540c7
eb78bd7
9d01d42
dcca5d6
6b48ba3
03909db
16c89eb
8984beb
5c13104
4d05210
f5d8ae0
82b9074
9053150
45b0029
6316cc6
297526c
37ac396
9e33a97
a0cc90a
cb3f4b0
a153ed4
6e3bc47
5b2c03a
e7b1144
5be7859
dbf4734
b5f1b67
5bcd5e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 the off-by-one possibility I mentioned, and I think it makes sense the way you have it. The intent of subtracting one is that it guarantees that:
Is that right?
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
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.
Can you come up with a test case?
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.
I need to think more about this. In hindsight, having stats functions return individual variables (or tuples) has made them difficult to extend in the future. I think we're going to want to return an object here. Is there anything else that object should contain (e.g. estimate of the quantile)? (This gets me thinking about whether this function should be actually be called
quantile
and basically be an extension ofnp.quantile
that returns not only an estimate of the underlying distribution's quantile but also a confidence interval.)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.
I don't have anything against returning an object (I quite like the idea actually). If we go down this way, then I would return an object called e.g. CI with pretty much all information. That is, with the following properties:
Regarding your suggestion to make this function an extension of
np.quantile
:I think it makes sense, but I fear it would make the function usage somehow blurry/hard to find. My thinking is, many people do not use CIs because they don't know much about them and it's not (always) easy to find an implementation to compute them. In order to fix that, I think having a function that is explicitly made for computing CIs is better than mixing everything into a global
quantile
function that would do everything (multi-dim, different interpolations for empirical quantile, etc.)Maybe more fundamentally,
np.quantile
deals with empirical quantiles, while here we aim to estimate distribution quantiles; now that I think about it this way, I become even more against merging the two. These two concepts are already confused enough :-)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.
OK. Maybe
scipy.stats.quantile
is not what we want, but I think we'll need to think about the interface a little more. We don't really have an established pattern for confidence intervals to follow, so we want to structure things in a way that can be extended.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.
I'm all in for that, and I'll gladly do the extra effort to make things more extendable. I'm just not most experience to judge/have an opinion on how that would look like.
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.
Can we use lower-case
lb
andub
for the things that are returned, and something else for the temporarylb
variable?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.
If these tests are from data this is published somewhere, so that we can confirm that these are the correct result, please list the reference.
If these tests don't confirm that
confint_quantile
is doing the right thing - just that it continues to do the same thing - then I would ask for stronger tests. Is there anything you can compare against?Also, please test your input validation. Can you think of any edge cases that might make sense to test that aren't specifically checked by input validation?
I would also prefer to see additional sanity checks - like, for a large number of samples, does the confidence interval get tighter? So could we do a test on, say,
np.linspace(0, 1, N)
for some arbitrary quantile and largeN
and observe that the upper and lower bound are close to that value?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.
Regarding the first point, I have not found something doing the exact same thing. This was discussed a bit above:
Do you think it would be worthwhile to include such a test?
Besides that, I can definitely add some more validation tests (that's the first time I wrote such code, I was not too sure how far I needed to go...). I will work on this and expand the test set.
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.
OK. Sometimes what I do, if possible, is write a minimalist implementation of the method right in the test suite (e.g. here). If you can boil it down to just a few lines that are easier to confirm (and if you write them on a different day, without copying and pasting so as not to be influenced by any possible mistakes above), it serves a bit like an independent check.
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, that should be fine. I do have a different implementation that I can use for this.
It was changed following earlier comments to leverage methods from the
binom
distribution. Anyhow, I see what I should do :-)