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

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

dohyunkim116
Copy link

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

@kose-y
Copy link
Member

kose-y commented Feb 25, 2024

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
Copy link
Author

dohyunkim116 commented Feb 25, 2024

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.

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.

3 participants