-
Notifications
You must be signed in to change notification settings - Fork 73
Add Bartlett's and Levene's tests for homogeneity of variances #216
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 Bartlett's and Levene's tests for homogeneity of variances #216
Conversation
Shimuuar
left a comment
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.
Thank you for PR! I'll be busy whole April and will need to read about both tests. So thing will probably move slowly.
There're few comments on things that caught my eye,
| -- Degrees of freedom and chi-squared distribution | ||
| df = k - 1 | ||
| chiDist = chiSquared df | ||
| pValue = mkPValue $ 1 - cumulative chiDist tStatistic |
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.
complCumulative should calculates 1-CDF without loss of precision
AFAIR χ² doesn't implement it but it may at some point
Statistics/Test/Levene.hs
Outdated
| zbari = map Sample.mean deviations | ||
| zbar = sum (zipWith (*) zbari (map fromIntegral ni)) / fromIntegral n | ||
|
|
||
| numerator = sum [fromIntegral (ni !! i) * (zbari !! i - zbar) ** 2 | i <- [0..k-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.
!! is generally smell since it's O(n). zbari and ni should probably be vector
|
Hi @Shimuuar! Thanks for the helpful feedback!
Thanks again, and no worries at all about things moving slowly — I really appreciate your time and review whenever you get a chance. |
Shimuuar
left a comment
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.
In general PR is fine.
complCumulative – I noticed that chiSquared currently doesn’t implement complCumulative. So for now, I’ve added a TODO comment in the code noting that this could be replaced with complCumulative when it becomes available.
It's not implemented so default implementation will be used instead. And if class method gets implemented it will be used automatically, no need to change anything in test implementation.
Please also add simple unit test in Tests.Parametric that checks both test compute approximately same test statistics and p-value as python/R/whatever.
Statistics/Test/Levene.hs
Outdated
| levenesTest :: Double -- ^ Significance level (alpha) | ||
| -> Center -- ^ Center calculation method | ||
| -> [V.Vector Double] -- ^ Input samples | ||
| -> Either String TestResult |
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.
Function should return Statistics.Test.Types.Test as Bartlett do,
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 have now reworked Levene's Test to return Test instead of TestResult.
Hi @Shimuuar! I've pushed the requested changes:
Please let me know if any further adjustments are needed! |
|
Well. It needs quite a bit of improvements but it would be easier to just merge it as is and fix it later |
Summary
Changes
Statistics.Test.BartlettStatistics.Test.Leveneexposed-modulessection in thestatistics.cabalfile.changelog.mdto reflect the addition.Notes for Reviewers