#217 Hypothesis test for two binomial (need review) #218
Conversation
nalimilan
left a comment
nalimilan
left a comment
There was a problem hiding this comment.
Thanks, and sorry for the delay in reviewing! This is an impressive PR. Would you still be interested in finishing this? I imagine this code is currently in use in ClinicalTrialUtilities.jl, right?
I have made mostly formal comments, I think the main work that remains to do is to integrate with the pattern used for all tests across the package, but it shouldn't be too hard.
| @@ -0,0 +1,541 @@ | |||
| # twobinomial.jl | |||
There was a problem hiding this comment.
Better put this in binomial.jl? This seems to be the pattern we use for other tests.
| Hypothesis type `htype` are: | ||
| - `:equality` | ||
| - `:equivalence` | ||
| - `:superiority` |
There was a problem hiding this comment.
How does this relate to/differ from the tail argument that is supported by existing tests?
| - `:riskratio` : risk ratio; | ||
| - `:oddratio` : odd ratio; | ||
|
|
||
| For each parameters can be used following methods. |
There was a problem hiding this comment.
The convention we use is to spell author names in full, separated with underscores. Also, can you describe each method and ensure consistency with BinomialTest when applicable? You can take inspiration from its docstring.
|
|
||
| export TwoSampleBinomialTest | ||
|
|
||
| struct TwoSampleBinomialTask |
There was a problem hiding this comment.
What's the advantage of storing this in a separate struct? I'd rather store these fields directly in TwoSampleBinomialTest.
| @@ -0,0 +1,88 @@ | |||
| using HypothesisTests, Test | |||
|
|
|||
| @testset "Two Binomial" begin | |||
There was a problem hiding this comment.
Have you checked all the values hardcoded here against other publications or software? Given the number of methods and special cases for each of them, I imagine these tests don't cover all branches, do they?
| end | ||
| ################################################################################ | ||
| function ci_diff(test; level, method) | ||
| if method == :mn || method == :default |
There was a problem hiding this comment.
Better drop :default and use :mn everywhere. Though for the current BinomialTest we use Clopper-Pearson as the default. Can we be consistent with that choice here?
| """ | ||
| TwoSampleBinomialTest(x1::Real, n1::Real, x2::Real, n2::Real, δ::Real; ptype::Symbol, htype::Symbol, alpha::Real = 0.05, method::Symbol = :default) | ||
|
|
||
| Perform a two sample binomial hypothesis check via computing confidence intrval |
There was a problem hiding this comment.
Also mention the meaning of each argument.
Thank you for reply and for all comments! :) Yes, I still interested in it, an I think I finish it, but it can take some time.
Yes, idea was to include code to HypothesisTests and use it from CTU and other packages. In some parts code is more optimized and was rethinked from last time. |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
=========================================
Coverage ? 92.82%
=========================================
Files ? 29
Lines ? 2064
Branches ? 0
=========================================
Hits ? 1916
Misses ? 148
Partials ? 0 Continue to review full report at Codecov.
|
|
try to make it step be step |
3 participants
Implementation for Superiority/Non-Inferiority, Equivalence, Equality interval hypothesis for proportion difference, odd ratio, risk ratio, issue #217.
Need review or/and validation.
Structures are discussible.