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

LDpred2: Removed counting of missing genotypes #183

Merged
merged 8 commits into from
Jun 14, 2023

Conversation

deepchocolate
Copy link
Contributor

@deepchocolate deepchocolate commented Jun 13, 2023

Fixes #181

Changes proposed in this pull request:

  • Removed countMissingGenotypes as it takes a significant amount of time of total execution time (~60%)
  • Clarified exception when there are missing genotypes

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

@deepchocolate deepchocolate changed the title Removed counting of missing genotypes and added exception handling LDpred2: Removed counting of missing genotypes Jun 13, 2023
@deepchocolate
Copy link
Contributor Author

Noted that bigstatsr::big_prodVec has an ncores argument, so I added parallelization here aswell

@espenhgn espenhgn added the enhancement New feature or request label Jun 14, 2023
@espenhgn
Copy link
Contributor

Thanks @deepchocolate;
Just a preliminary report; some test(s) is failing (ran git clean -fd . in the tests directory beforehand):

$ cd tests && git clean -fd . && cd test_LDpred2
$ bash run.sh
...
Test appending score to output file
Runing unittests on output
-- Failure (???): Appended score output ----------------------------------------
hasAllColumns(tmp, cnamesExp) is not TRUE

`actual`:   FALSE
`expected`: TRUE 
Expected columns FID,IID,score,newScore but FID,IID,score were found in /nrec/space/espenh/containers/tests/test_LDpred2/output/public-data.score.inf

-- Failure (???): Appended score output ----------------------------------------
file.exists(filePlot) is not TRUE

`actual`:   FALSE
`expected`: TRUE 
Diagnostic plot was not found: /nrec/space/espenh/containers/tests/test_LDpred2/output/newScore.png

Error in reporter$stop_if_needed() : Test failed
Calls: test_that -> <Anonymous>
Execution halted

Can you reproduce it on your end?

I've updated the version and stuff for the next release.

@deepchocolate
Copy link
Contributor Author

Thanks @deepchocolate; Just a preliminary report; some test(s) is failing (ran git clean -fd . in the tests directory beforehand):

$ cd tests && git clean -fd . && cd test_LDpred2
$ bash run.sh
...
Test appending score to output file
Runing unittests on output
-- Failure (???): Appended score output ----------------------------------------
hasAllColumns(tmp, cnamesExp) is not TRUE

`actual`:   FALSE
`expected`: TRUE 
Expected columns FID,IID,score,newScore but FID,IID,score were found in /nrec/space/espenh/containers/tests/test_LDpred2/output/public-data.score.inf

-- Failure (???): Appended score output ----------------------------------------
file.exists(filePlot) is not TRUE

`actual`:   FALSE
`expected`: TRUE 
Diagnostic plot was not found: /nrec/space/espenh/containers/tests/test_LDpred2/output/newScore.png

Error in reporter$stop_if_needed() : Test failed
Calls: test_that -> <Anonymous>
Execution halted

Can you reproduce it on your end?

I've updated the version and stuff for the next release.

Are you sure this is not a memory issue again? Because when I ran it, it stopped at the change in scripts/extended.sh, it should stop there if it doesn't receive an error. Don't know why I changed that.

When I fixed that and removed all output files all test passed

@espenhgn espenhgn mentioned this pull request Jun 14, 2023
4 tasks
@espenhgn
Copy link
Contributor

Yes, I think it is due to lack of memory (during Loading LD reference from /ldpred2_ref/ldref_hm3_plus/LD_with_blocks_chr@.rds LD_with_blocks_chr1.rds : loading LD for 3495 out of 117534 SNPs, but the reason is not captured).
Will check with @ofrei what we can do to upgrade the specs to 16GB of RAM for this VM we're using.
I'm fine with merging as is until that is resolved.

@deepchocolate
Copy link
Contributor Author

Yes, I think it is due to lack of memory (during Loading LD reference from /ldpred2_ref/ldref_hm3_plus/LD_with_blocks_chr@.rds LD_with_blocks_chr1.rds : loading LD for 3495 out of 117534 SNPs, but the reason is not captured). Will check with @ofrei what we can do to upgrade the specs to 16GB of RAM for this VM we're using. I'm fine with merging as is until that is resolved.

I'm guessing the system terminates the execution somehow so that there's no error returned
Just going to switch back the exit code in extended.sh and then I'll merge

@deepchocolate deepchocolate merged commit 960ffb2 into main Jun 14, 2023
@deepchocolate deepchocolate deleted the deepchocolate/ldpred2_time_thief branch June 14, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LDpred2: Remove time thief
2 participants