Skip to content

Conversation

@kescobo
Copy link
Collaborator

@kescobo kescobo commented Dec 11, 2020

This is mostly straightforward. I haven't fixed up the tests yet, but the only thing not working from functions in the README is in this method: thresholds(alt::NoncentralT) = -alt.ncp, alt.ncp, where NoncentralT distribution no longer has ncp

   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.0-DEV.1668 (2020-12-04)
 _/ |\__'_|_|_|\__'_|  |  Commit 0253ebf147 (7 days old master)
|__/                   |

julia> using PowerAnalysis, HypothesisTests

julia> n = 100
100

julia> δ = 0.01
0.01

julia> σs = 1.0
1.0

julia> p = 0.8
0.8

julia> α = 0.05
0.05

julia> power(EqualVarianceTTest, n, δ, σs, α)
0.05056741592646641

julia> effect_size(EqualVarianceTTest, n, p, α)
0.39813813756823213

julia> sample_size(EqualVarianceTTest, δ, σs, p, α)
156978.1705208713

julia> type_1(EqualVarianceTTest, n, δ, σs, α)
0.05

julia> type_2(EqualVarianceTTest, n, δ, σs, α)
0.9494325840735336

julia> type_s(EqualVarianceTTest, n, δ, σs, α)
0.41847831065271157

julia> type_m(EqualVarianceTTest, n, δ, σs, α)
ERROR: type NoncentralT has no field ncp
Stacktrace:
 [1] getproperty(x::Distributions.NoncentralT{Float64}, f::Symbol)
   @ Base ./Base.jl:33
 [2] thresholds(alt::Distributions.NoncentralT{Float64})
   @ PowerAnalysis ~/Repos/PowerAnalysis.jl/src/t_test.jl:50
 [3] type_m(::Type{EqualVarianceTTest}, ns::Int64, δ::Float64, σs::Float64, α::Float64, one_sided::Bool)
   @ PowerAnalysis ~/Repos/PowerAnalysis.jl/src/generics.jl:180
 [4] type_m(::Type{EqualVarianceTTest}, ns::Int64, δ::Float64, σs::Float64, α::Float64)
   @ PowerAnalysis ~/Repos/PowerAnalysis.jl/src/generics.jl:170
 [5] top-level scope
   @ REPL[14]:1

julia> exaggeration_factor(EqualVarianceTTest, n, δ, σs, α)
ERROR: type NoncentralT has no field ncp

The type signature changed from here

immutable NoncentralT <: ContinuousUnivariateDistribution
    df::Float64
    ncp::Float64
    function NoncentralT(d::Real, nc::Real)
    	d >= zero(d) && nc >= zero(nc) || error("df and ncp must be non-negative")
        new(float64(d), float64(nc))
    end
end

to here

struct NoncentralT{T<:Real} <: ContinuousUnivariateDistribution
    ν::T
    λ::T
    NoncentralT{T}(ν::T, λ::T) where {T} = new{T}(ν, λ)
end

So is ncp just the new λ?

Still needs:

  • What's ncp?
  • fix tests syntax
  • Add test and docs bots

I'm happy to keep going here, though if you're not interested in holding onto ownership, maybe it could be migrated to the JuliaStats org?

@kescobo
Copy link
Collaborator Author

kescobo commented Dec 11, 2020

This might close #1 also

@kescobo
Copy link
Collaborator Author

kescobo commented Dec 14, 2020

All tests pass now, but the last command in the README fails

julia> exaggeration_factor(EqualVarianceTTest, n, δ, σs, α)
full precision may not have been achieved in 'pnt{final}'
full precision may not have been achieved in 'pnt{final}'
ERROR: MethodError: no method matching gauss(::PowerAnalysis.var"#f1#12"{Distributions.NoncentralT{Float64}}, ::Float64, ::Float64; rtol=1.0e-8)
Closest candidates are:
  gauss(::Any, ::Any, ::Real, ::Real; rtol, quad) at /home/kevin/.julia/packages/QuadGK/czbUH/src/weightedgauss.jl:60
  gauss(::Integer, ::Real, ::Real) at /home/kevin/.julia/packages/QuadGK/czbUH/src/gausskronrod.jl:110 got unsupported keyword argument "rtol"
  gauss(::Type{T}, ::Integer, ::Real, ::Real) where T<:AbstractFloat at /home/kevin/.julia/packages/QuadGK/czbUH/src/gausskronrod.jl:111 got unsupported keyword argument "rtol"

@kescobo kescobo changed the title [WIP] Update to julia 1 Update to julia 1 Dec 14, 2020
@kescobo
Copy link
Collaborator Author

kescobo commented Dec 14, 2020

Alright, aside from that issue above, I think this is good to go. There's still plenty of work to make this more idiomatic for modern julia, but this is at least functional.

@johnmyleswhite
Copy link
Owner

So abstractly I'm happy to merge this and turn over ownership to you, but would like some time to go over everything. Ok to follow up next week? Also happy to set up a quick chat over Zoom -- I find using Github isn't a great forum for communication for something with my schedule in which I can block out 30 minutes for deep discussion, but struggle with async comms.

@kescobo
Copy link
Collaborator Author

kescobo commented Dec 14, 2020

Yes, definitely fine to take some time, and yes, I'd be happy to meet up via zoom. No rush at all.

I'm US east coast time and I typically have a lot of open space in the schedule on Tuesday/Thursday, and I can be reached on slack/zulip/discourse or via email linked to my GH account.

@johnmyleswhite
Copy link
Owner

How about we chat on Tuesday next week at 11 AM Eastern?

@johnmyleswhite
Copy link
Owner

I'll send an e-mail with more details.

@kescobo
Copy link
Collaborator Author

kescobo commented Dec 22, 2020

Just summarizing call before merging so I at least have some reference when I forget about this later.

  • John has memory of some additional tests that were written, but can't locate them at the moment. I might receive a code dump at some point that I can integrate
  • This code should be functional, but there may be extreme examples where some methods fall down, particularly calls to fzero (eg here) for reasons possibly related to numerical instability.
  • John is OK with me taking over ownership / maintenance
  • Also OK with moving into JuliaStats org if they agree

If I misunderstood anything @johnmyleswhite , feel free to correct. Thanks again for meeting!

@kescobo kescobo merged commit df7c7d3 into johnmyleswhite:master Dec 22, 2020
@kescobo kescobo deleted the julia1 branch December 22, 2020 16:19
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.

2 participants