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
Shapiro-Wilk normality test #124
Shapiro-Wilk normality test #124
Changes from 59 commits
6e2a5b0
c576f60
0e52c97
dc74a53
ee9b8aa
23485c9
8219f5e
e843ca5
9f1bb32
52ba998
5669f2c
c2eda86
b956bf0
4116979
875cc80
a8838c6
240187f
4953630
57e76e6
48783ff
2dbc09b
5660d69
771096d
a15c906
0133ca3
31ea237
9891972
714f494
060e54e
c162b49
d013864
c7848f1
e07b4aa
103bb20
820b62c
8afe009
1db096b
96d634a
442561f
282ef0b
0460aa0
2e408fb
cfad621
080af1b
edf696f
fcf00ef
4b5e2d8
e874016
5be7f67
0af19f2
fe0edc9
47bbd04
3a4d376
55623bd
7abe439
d36355a
23318ce
cccb03c
475498d
135e43a
833a18e
6403297
262bf2c
46e4cca
cf21de0
1bd9d79
6047be0
6e313c7
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 not needed, it seems?
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.
Why do you think so @devmotion? to me it seems very much needed
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.
Because you create
m
as aVector{Float64}(undef, N)
a few lines above.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 but then I resize it to length
n = N÷2
, and compute only the first half (normalizing w.r.t.n
). Only then I fill the rest of the vector below line 62; To do so I need to bring back the old sizeThere 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 missed this. Can you remove all
resize!
calls? I think you should be able to work with aview
instead of resizing the array.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,
@view
s are much better than resizes ;)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.
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.
@devmotion isn't
moment
normalized (i.e. integrated w.r.t. uniform measure onX
? Then this should beS² = moment(X,2)*length(X)
.But the real question is: did you benchmark this? On my machine this is (2-6) × slower (N ∈ 2.^7:15, increasing with N) than before.
(Which, accidentally, makes it even slower than the non-BLAS dot for old SWCoeffs (N=2^7, increases to ~8 for N=2^15)
If we really care about perf I'd rather revert this.
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.
We should improve
moment
if it is slow. But regardless, you're right of course that themoment
will be normalized. I'm fine with reverting it.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.
What is the utility of allowing the user to compute the Shapiro-Wilk test statistic at all if they can't get its p-value nor view the object directly in the REPL (since
show
callspvalue
)? That's an honest question; I would guess that many users would try it and get frustrated without checking the docstring but that is a naive assumption.I'd probably recommend commenting out the censoring-related functionality for the time being, then someone can implement it fully in a follow-up PR. Thoughts on that?
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.
@ararslan feel free to suggest (via gh suggestions) any edits that would suit your proposal
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 guess it was half question, half proposal—do you envision a use case for this with censored data as it is currently implemented in this PR (no
pvalue
/show
)? If not, I can make those suggestions.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.
Well the
W
statistic can be still useful if somebody wants to use section 3.3 of Royston 1993 to calculate significance levels directly.I didn't implement those methods back then since the significance levels there are only empirical, and until I need to do it at work I don't see myself doing this now either ;)
If those suggestions push this PR over the final line, please make them!