xKDR / Survey.jl

Analysis of complex surveys

https://xkdr.github.io/Survey.jl/

GNU General Public License v3.0

53 stars 19 forks source link

WIP: Add JKn replicate weights #236

Closed RohitRathore1 closed 1 year ago

RohitRathore1 commented 1 year ago

It resolves issue #230

codecov-commenter commented 1 year ago

Codecov Report

Merging #236 (97c6b06) into main (82a954f) will decrease coverage by 9.23%. The diff coverage is 0.00%.

@@             Coverage Diff             @@
##              main     #236      +/-   ##
===========================================
- Coverage   100.00%   90.77%   -9.23%     
===========================================
  Files           12       13       +1     
  Lines          187      206      +19     
===========================================
  Hits           187      187              
- Misses           0       19      +19

Impacted Files	Coverage Δ
src/jackknife.jl	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

smishr commented 1 year ago

Hi @RohitRathore1. Thanks for contributing to the project. I saw the main change you made from fill((nh - count(==(i), randinds)) . Can your verify from section 2/2.1 of this replicate methods paper that this is indeed the correct algorithm for JKn?

I'll look and test further in coming days.

Also, please add some relevant test examples, in test/jackknife.jl as we have already reached 100% code coverage, and want to keep that level. A better docstring and a doctest would also be nice for main branch

RohitRathore1 commented 1 year ago

Hi @RohitRathore1. Thanks for contributing to the project. I saw the main change you made from fill((nh - count(==(i), randinds)) . Can your verify from section 2/2.1 of this replicate methods paper that this is indeed the correct algorithm for JKn?

I'll look and test further in coming days.

Also, please add some relevant test examples, in test/jackknife.jl as we have already reached 100% code coverage, and want to keep that level. A better docstring and a doctest would also be nice for main branch

Thank you @smishr! I will add the tests. According to me, the algo is correct but your review will be appreciated.

ayushpatnaikgit commented 1 year ago

For the test case, make a replicate design in R using JKn. Calculate the mean or total and check if the standard error matches Julia for the same steps.

RohitRathore1 commented 1 year ago

For the test case, make a replicate design in R using JKn. Calculate the mean or total and check if the standard error matches Julia for the same steps.

@ayushpatnaikgit @smishr thanks for the review. I will make the required changes.

ayushpatnaikgit commented 1 year ago

Shall we close this? @RohitRathore1 can you do a new one?

RohitRathore1 commented 1 year ago

Shall we close this? @RohitRathore1 can you do a new one?

Hey sorry, I was having issues with my system. Please can you reopen this PR or should I create a new PR?

smishr commented 1 year ago

Shall we close this? @RohitRathore1 can you do a new one?

Hey sorry, I was having issues with my system. Please can you reopen this PR or should I create a new PR?

i have reopened the Pr. Please do into v0.1.1 branch, not main

smishr commented 1 year ago

@RohitRathore1 The JKn algorithm is deterministic (not stochastic) so random numbers are not used at all in it (using Random is not needed), and all lines using rng or randints or Mersenne twister are incorrect. The algo defines the number of replicates as one less than the strata, so replicates is a fixed number (not an input argument to the function) as given in section 2.1 here. In essence, Jackknife algorithm is very similar to "Leave one out" Cross Validation algorithm (LOOCV) popular in ML.

smishr commented 1 year ago

transferring this to #260 as more work has happened there