-
Notifications
You must be signed in to change notification settings - Fork 19
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
WIP: Abstract type for summary statistics (Mean) #277
Conversation
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.
The main goal is to have an easy type dispatching over all the SummaryStats Functions. We need to check how Function
types heirarchy works in Julia. There should be AbstractFunction
and mean
, total
etc can be sub types of (<:) SummaryStats::AbstractFunction
|
||
""" | ||
|
||
abstract type SummaryStats end |
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 this abstract type looks good
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.
Am wondering if SummaryStats
should be an AbstractFunction
instead?
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.
Didn't know about this, will look into it !
|
||
""" | ||
|
||
function confint(estimate::Float64, std_dev::Float64; type::String="normal", alpha::Float64=0.05) |
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.
EDIT: After reviewing I think you can just copy the entire of _ci
from 190 and rename it here.
- I think
estimate
andstd_dev
should be typeAbstractVector
(like in_ci
in WIP: Add generalCI
and integration withmean
#190 ). This way you can pass whole column of estimates at the same time. - I would keep
confint
in a different fileconfint.jl
.
design::SurveyDesign | ||
end | ||
|
||
function Mean(x::Symbol, design::SurveyDesign) |
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 should be an addition to the existing
mean.jl
file. You can add with multiple dispatch you can definemean(x::Symbol, design::ReplicateDesign, ci_type=
normal...)
see WIP: Add generalCI
and integration withmean
#190 line 169-170 of what the defintion of function can be - build for
ReplicateDesign
first, sinceSurveyDesign
doesnt have std error yet
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.
- to your first point : see my reply to your comment on line 60
- to your 2nd point : yes that makes sense
|
||
""" | ||
|
||
struct Mean <: SummaryStats |
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 should be done as part of edits inside
mean.jl
- Im not sure whether the struct would just need two things x and design
- I have gut feeling that
Mean
should be a Function, not a struct. Lets ask @ayushpatnaikgit 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.
I went with the idea that it's easier to have all summary statistics as a struct in a single file (SummaryStats.jl
) since it's easier to manage, if we want to add another statistic later on you would just add it to this one file.
The reason for having Mean
as a struct (as well as a Function) is the same : if we want to add other statistics, and we want all statistics to have a set of basic elements (e.g. an estimate), you would write this into SummaryStats
which is the "parent abstract type" and it would be automatically passed down to all the other statistics (since Mean <: SummaryStats
). At least this is my understanding.
Also mean.jl
(and quantile, ratio, total) has a lot of dispatched functions, so I thought it'd be more efficient and maintainable to have only 1 file which implements all summary stats, instead of 1 file per stat (mean.jl
, quantile.jl
etc) which has multiple dispatched functions inside.
I agree with having confint.jl
separately though.
Thoughts ?
Have you considered how this would work with domain estimation? Where we have |
Overall, I think it's a good idea to be able to do It will need a few show methods to make it beautiful. |
I suggest having struct Estimate
value::Number
SE::Number
end Instead of
For domain estimation, we'll need a separate struct struct DomainEstimates
values::Vector{Number}
SE::Vector{Number}
end After this, we can have a function confint(x::Estimate). |
How does this broadcast to estimates which are vectors? |
Any reason not to use https://github.com/JuliaPhysics/Measurements.jl here? It's fundamentally the same data structure but probably supported more widely across the ecosystem. In this case a vector estimate would be a vector of |
Codecov Report
@@ Coverage Diff @@
## next_release #277 +/- ##
================================================
- Coverage 100.00% 95.52% -4.48%
================================================
Files 12 14 +2
Lines 200 268 +68
================================================
+ Hits 200 256 +56
- Misses 0 12 +12
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Ayush and I looked at the Measurements.jl package, we see where you are coming from. Except for the pretty printing of uncertainty, their class would not be useful for other aspects of Survey.jl. But some ideas can be adapted from them |
Resolves #275, #184, #274
Partial implementation of @smishr's confidence interval function
Implemented Mean structure as an abstract SummaryStat
Remaining to do on this PR :
Can implement Total, Quantile, and Ratio later