Internal qsturng Implementation Substitution #156
Conversation
Codecov Report
@@ Coverage Diff @@
## master #156 +/- ##
=======================================
Coverage 99.38% 99.38%
=======================================
Files 18 18
Lines 3239 3239
Branches 531 531
=======================================
Hits 3219 3219
Misses 8 8
Partials 12 12
Continue to review full report at Codecov.
|
raphaelvallat
added
feature request 🚧
|
Thanks @DominicChm, that's really great! A few minor comments before merging:
|
|
Ok, @raphaelvallat, I've updated examples and changelog in a way that I hope is consistent with previous ones. I added the change to the 0.3.9 (current?) version because I'm not sure about when this change should ship, but could start another section instead if it makes more sense. |
|
Amazing @DominicChm ! So, we actually need to start a new section, I think let's go for 0.3.10 as I'm hoping to release this bugfix soon. Thanks again! |
|
@raphaelvallat No problem, happy to help! I've added the new changelog section. I included the same disclaimer from the previous bugfix because it seems like this problem could significantly affect p-values. |
docs/changelog.rst
Outdated
| @@ -15,6 +15,8 @@ We therefore strongly recommend that all users UPDATE Pingouin (:code:`pip insta | |||
|
|
|||
| Furthermore, and to prevent a similar issue, we have now disabled ``marginal=False`` in two-way repeated measure design. As of this release, ``marginal=False`` will therefore only have an impact on the between-factor T-test(s) of a mixed design. | |||
|
|
|||
| In addition, the internal function responsible for generating a significance Q value for :py:func:`pingouin.pairwise_ttests` has been replaced with an equivalent from statsmodels, as it was found to be substantially inaccurate for certain combinations of treatments, degrees of freedom, and significances. | |||
There was a problem hiding this comment.
-
The changes proposed in this PR only affects the pairwise_tukey and pairwise_gameshowell functions, not the pairwise_ttests function
-
Could you add a hyperlink to the PR, as well as your GitHub user name (if you wish)?
There was a problem hiding this comment.
I think we can also be more exhaustive and say that there was an error in the algorithm for the studentized range approximation that lead to minor errors in the p-values of the pairwise_tukey / pairwise_gamesshowell function. Most likely however, this did not affect the significance levels, with the exception of the p-values that may have been very close to significance (around the 0.05 threshold) -- or am I wrong?
|
Merging now, thanks again! |
This PR resolves #155 by removing the internal
qsturngandpsturngfunction implementations (used for tukey's pairwise comparison) and replacing them with statsmodels' version.The below is a worst-case example of the behavior seen with the current version. Pingouin's error compared to Statsmodels' is much greater than it should be, considering that the implementation of their
psturngandqsturngfunctions is almost identical. With this change, that implementation is replaced with Statmodels' more accurate one.Note: This graph is of the error of various different functions at varying degrees of input precision. In this case, statmodels' and pingouin's lines are generated by calling their
psturngfunctions with the labeled input parameters, then subtracted from the high-precision numerical result of the integral thepsturngfunction emulates to get error.