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
Use
StatsAPI.pvalue
+StatsAPI.HypothesisTest
#297Use
StatsAPI.pvalue
+StatsAPI.HypothesisTest
#297Changes from all commits
5ae34a2
cf1f480
d83c2fd
b20efcd
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.
Why change 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.
Initially it seemed as if we could get rid of the StatsBase dependency completely. It turned out that this is not the case but the five functions loaded below are needed +
StatsBase.PValue
for printing. Nevertheless, as with the imports of Combinatorics and Rmath in the lines below, explicitly importing these five functions seemed a bit clearer.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 personally find listing out every symbol to be particularly clarifying or useful but I know not everyone shares that opinion. :) In this case I'd prefer not to change the StatsBase bit as part of this PR as it seems like an unrelated change but I don't feel too strongly.
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 a completely strong opinion on this matter either. I wasn't 100% sure what changes you had in mind exactly, but I reverted this line and some related changes below. Is it better now?
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.
It looks ✨ a m a z i n g ✨, apologies for the pedantry and for the delay, this got lost amidst a bunch of other notifications
This file was deleted.