Skip to content
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

Implement StatsAPI.pvalue #1719

Merged
merged 4 commits into from
May 9, 2023
Merged

Implement StatsAPI.pvalue #1719

merged 4 commits into from
May 9, 2023

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented May 8, 2023

Currently pvalue(::UnivariateDistribution, ...) is defined in HypothesisTests and owned by HypothesisTests. However, when switching HypothesisTests to StatsAPI 1.6 which defines StatsAPI.pvalue such definitions would be type piracy. Hence this PR adds them to Distributions.

Note that StatsAPI is already an indirect dependency of Distributions via StatsBase, so this PR does not add packages to the dependency tree.

TODO:

  • Add tests

test/runtests.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (d460846) 85.82% compared to head (d73145d) 85.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1719      +/-   ##
==========================================
+ Coverage   85.82%   85.85%   +0.03%     
==========================================
  Files         137      138       +1     
  Lines        8318     8337      +19     
==========================================
+ Hits         7139     7158      +19     
  Misses       1179     1179              
Impacted Files Coverage Δ
src/Distributions.jl 100.00% <ø> (ø)
src/statsapi.jl 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

test/statsapi.jl Outdated Show resolved Hide resolved
@@ -0,0 +1,26 @@
using Distributions
using StatsAPI: pvalue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have Distributions reexport pvalue? I'd be generally in favor but don't feel too strongly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this break HypothesisTests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not after HypothesisTests extends pvalue from StatsAPI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but all existing releases of HypothesisTests would suddenly be broken, wouldn't they? Certainly, this could be fixed by modifying the registry but I think this makes it less attractive to re-export pvalue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge and release without re-exporting pvalue for now but this can be reconsidered and changed in a subsequent release if desired.

Co-authored-by: Alex Arslan <ararslan@comcast.net>
@devmotion devmotion merged commit 4fd8be8 into master May 9, 2023
@devmotion devmotion deleted the dw/statsapi branch May 9, 2023 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants