search

OpenMendel / SnpArrays.jl

Compressed storage for SNP data
https://openmendel.github.io/SnpArrays.jl/latest
Other
44 stars 9 forks source link

Fix a minor error with standardization of SNPs when copying #138

Open dohyunkim116 opened 7 months ago

dohyunkim116 commented 7 months ago

Centering and scaling were not being performed when copying SnpLinAlg into a numeric matrix with mean imputation.

kose-y commented 7 months ago

I'm not sure about the point of this PR. Could you explain more about it? The original code imputes the missing value with the column mean, and the proposed code imputes the missing value with zero when s.impute is true. See the output from the unit tests.

dohyunkim116 commented 7 months ago

I think the modified code does impute with column mean, and return zero when s.center is true. The original code returned column mean regardless s.center was set to true. Mean imputation should always return zero after centering, right?

In the below test, (lines 288, 299 and 300 in runtests.jl), 

mousela = SnpLinAlg{t}(mouse, model=ADDITIVE_MODEL, center=true, scale=true, impute=true)
 v = copyto!(zeros(1), @view(mousela[702])) # missing entry
 @test isapprox(v[1], 1.113003134727478, atol=1e-6)

we are checking whether the mean imputation is performed and is approximately equal to 1.113. The test should actually check whether v is equal to zero since center = true. If we have set center = false, then the test in line 300 should pass. Please correct me if I am missing something here.