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

bydomain is hardcoded for bootstrap variance #286

Open
smishr opened this issue Mar 23, 2023 · 5 comments
Open

bydomain is hardcoded for bootstrap variance #286

smishr opened this issue Mar 23, 2023 · 5 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@smishr
Copy link
Contributor

smishr commented Mar 23, 2023

See line 22-27 in by.jl

    for i = 1:nd
        filtered_dx = filter(!isnan, Xt_mat[i, :] .- X.statistic[i])
        push!(ses, sqrt(sum(filtered_dx .^ 2) / length(filtered_dx)))
    end
    replace!(ses, NaN => 0)
    X.SE = ses

This is hardcoding the variance for "bootstrap" type ReplicateDesign. For future additions, like JKn, the variance is calculated differently.

TODO: Update bydomain function with modularity for current and future methods of variance estimation.

@smishr smishr added bug Something isn't working enhancement New feature or request labels Mar 23, 2023
@ayushpatnaikgit
Copy link
Member

ayushpatnaikgit commented Mar 23, 2023

We need to update all the functions and think carefully. How about:

abstract type ReplicateDesign end 

struct BootstrapDesign <: ReplicateDesign
       end

struct JackknifeDesign <: ReplicateDesign
       end

?

Then we can do multiple dispatch instead of if else ladders.

I wish there was inheritance. We could have done BoostrapDesign <: ReplicateDesign <: SurveyDesign

@smishr
Copy link
Contributor Author

smishr commented Mar 25, 2023

We need to update all the functions and think carefully. How about:

abstract type ReplicateDesign end 

struct BootstrapDesign <: ReplicateDesign
       end

struct JackknifeDesign <: ReplicateDesign
       end

?

Then we can do multiple dispatch instead of if else ladders.

I wish there was inheritance. We could have done BoostrapDesign <: ReplicateDesign <: SurveyDesign

Yes something like this should improve the modularity of the code.

But we need to throughly think this through. All other parts of the package should be made modular as well at the same time

How do other packages solve the inheritance issues?

@asinghvi17
Copy link
Member

asinghvi17 commented Mar 25, 2023

I haven't looked through the code too much, but a partial solution presents itself:

abstract type AbstractSurveyDesign end

struct SurveyDesign <: AbstractSurveyDesign
...
end

abstract type AbstractReplicateDesign <: AbstractSurveyDesign end

struct BootstrapDesign <: AbstractReplicateDesign
       end

struct JackknifeDesign <: AbstractReplicateDesign
       end

so you basically introduce a top-level abstract type, which SurveyDesign and AbstractReplicateDesign both subtype. Then, generic methods can be dispatched on AbstractSurveyDesign. I've used this pattern, and seen it used, in a number of places (most notably as Number -> (Real -> ..., Complex)).

@smishr
Copy link
Contributor Author

smishr commented Mar 27, 2023

I haven't looked through the code too much, but a partial solution presents itself:

abstract type AbstractSurveyDesign end

struct SurveyDesign <: AbstractSurveyDesign
...
end

abstract type AbstractReplicateDesign <: AbstractSurveyDesign end

struct BootstrapDesign <: AbstractReplicateDesign
       end

struct JackknifeDesign <: AbstractReplicateDesign
       end

so you basically introduce a top-level abstract type, which SurveyDesign and AbstractReplicateDesign both subtype. Then, generic methods can be dispatched on AbstractSurveyDesign. I've used this pattern, and seen it used, in a number of places (most notably as Number -> (Real -> ..., Complex)).

@ayushpatnaikgit @codetalker7 what do you guys think here?

@asinghvi17
Copy link
Member

Ah, I just realized I suggested two different approaches :D - let's discuss on the call if any of those make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants