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: Effective sample size, cores used, hyperparameters #173

Merged
merged 23 commits into from
May 19, 2023

Conversation

deepchocolate
Copy link
Contributor

@deepchocolate deepchocolate commented May 13, 2023

Fixes #149 and #175

Changes proposed in this pull request:

  • snp_ldsc uses multiple cores
  • Effective sample size can be provided as nr cases (--n-cases) and nr controls ( --n-controls)
  • Maximum of hyper parameter p can be provided (--hyper-p-max) to snp_ldpred2_auto
  • Diagnostic plots named after --name-score if given

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 marked this pull request as ready for review May 15, 2023 09:41
MINOR version when you add functionality in a backward compatible manner
@deepchocolate
Copy link
Contributor Author

@espenhgn @ofrei I discovered a problem with using --out-merge in that the diagnostic plots will replace each other. Will fix #175 in this PR aswell. I suggest using the --name-score argument for that purpose.

@espenhgn
Copy link
Contributor

@espenhgn @ofrei I discovered a problem with using --out-merge in that the diagnostic plots will replace each other. Will fix #175 in this PR aswell. I suggest using the --name-score argument for that purpose.

Ok. I looked through the rest of this PR as is, and looks good to me. Just waiting for the test script to finish.

@espenhgn
Copy link
Contributor

Don't know if this PR introduced this test crash with the LDpred2 test script:

...
Test appending score to output file
Runing unittests on output
-- Failure (???): Appended score output ----------------------------------------
hasAllColumns(tmp, cnamesExp) is not TRUE

`actual`:   FALSE
`expected`: TRUE 

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

Do you also see this?

@deepchocolate
Copy link
Contributor Author

Don't know if this PR introduced this test crash with the LDpred2 test script:

...
Test appending score to output file
Runing unittests on output
-- Failure (???): Appended score output ----------------------------------------
hasAllColumns(tmp, cnamesExp) is not TRUE

`actual`:   FALSE
`expected`: TRUE 

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

Do you also see this?

Ah, yes! It's because I removed running both the auto and inf mode in that part of the tests.
I didn't see it because I only ran the extended.sh tests the last time which I thought was sufficient.
Will fix!

@deepchocolate
Copy link
Contributor Author

@espenhgn Turned out to be the memory issue again as termination due to memory is not captured as an error.. My impression from watching the process is that memory usage peaks at about 8GB.

@espenhgn
Copy link
Contributor

Ok. If you think memory is the issue, maybe then there is not so much I can do to run all tests reliably on this VM I'm using. The usual situation is that there are only 3-4GB of RAM available for actual use, when background processes are running:
image
But maybe I'm also causing more issues by using VSCode to connect/edit directly as many small processes are created.

Copy link
Contributor

@espenhgn espenhgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The test crash I encountered appears to be memory related

@deepchocolate deepchocolate linked an issue May 19, 2023 that may be closed by this pull request
@deepchocolate
Copy link
Contributor Author

@espenhgn Added the fix for #175 if you wish to inspect

@espenhgn
Copy link
Contributor

@espenhgn Added the fix for #175 if you wish to inspect

I trust you to merge this :)

@deepchocolate deepchocolate merged commit 3a8d1f7 into main May 19, 2023
@deepchocolate deepchocolate deleted the deepchocolate/ldpred2_parameters branch May 19, 2023 13:13
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.

LDpred2: --out-merge does not produce diagnostic plots correctly LDpred2: Minor issues and improvements
2 participants