check if there are no p values to correct #233
Conversation
gbordyugov
requested review from
shansfolder,
daryadedik and
aaron-mcdaid-zalando
gbordyugov
changed the title
expan/core/correction.py
Outdated
| :return: new critical value (i.e. the corrected alpha) | ||
| :rtype: float | ||
| """ | ||
| if not original_p_values: |
There was a problem hiding this comment.
A few challenges from me ;)
In which cases will we pass an empty list of p values here?
Did you discover this problem from a real experiment bug?
Do we want to raise an exception or just log the error?
Thank you!
There was a problem hiding this comment.
@shansfolder that's something that you explained to me: if there are statistical tests with too few samples, they're marked as invalid and the overall number of statistical tests is decreased. So if all statistical tests in a statistical test suite are invalid, we wind up with having zero statistical tests to analyse (and zero original p values).
I haven't thought know about the logging yet.
There was a problem hiding this comment.
@shansfolder this is a great question!
We need to add input parameter validation in the call stack up to entry methods in expan lib (Experiment class I guess)
There was a problem hiding this comment.
@gbordyugov I believe assert instruction is a bit more self-explanatory to check methods' pre-, post- conditions.
But I don't really mind, feel free to use it if you agree.
There was a problem hiding this comment.
I see. Thank you.
That's a log to me (otherwise we should raise the exception earlier when we find that there is no valid tests in the suite).
There was a problem hiding this comment.
@shansfolder for me, that's not just a log - it's unrecoverable. If there are no p-values to correct, we cannot return a sensible corrected alpha threshold.
This tries to handle the lack of p-value in a slightly more elegant way.