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

Compatibility with custom dataset #58

Merged
merged 50 commits into from
Dec 4, 2023
Merged

Compatibility with custom dataset #58

merged 50 commits into from
Dec 4, 2023

Conversation

roskamsh
Copy link
Collaborator

This branch was created to make the targene-pipeline compatible with datasets that are not the UKBB.

@olivierlabayle
Copy link
Member

olivierlabayle commented Feb 27, 2023

@roskamsh Hello there! How are we doing around here? Is it ready for review? When ready you can add me as a reviewer on all your PRs :)

@roskamsh
Copy link
Collaborator Author

@olivierlabayle this should be ready to review. The only thing I need to do is update the docs regarding how the setup slightly changes in regards to parameters in the nextflow.config but I believe that is on a separate branch?

@olivierlabayle
Copy link
Member

@olivierlabayle this should be ready to review. The only thing I need to do is update the docs regarding how the setup slightly changes in regards to parameters in the nextflow.config but I believe that is on a separate branch?

Amazing I'll start to look at the code then ! Ideally, we can do it on the same branch so that the PR is self contained.

modules/genotypes.nf Outdated Show resolved Hide resolved
Copy link
Member

@olivierlabayle olivierlabayle left a comment

Choose a reason for hiding this comment

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

Thank you Breeshey for your work here. Appart from my comment on the script this looks good to me and the tests pass. Before merging we need to add one test for this new scenario and add some documentation. Happy to discuss that further :)

Todo:

  • Add test
  • Update docs

@roskamsh
Copy link
Collaborator Author

To-do checklist has now been completed.

Copy link
Member

@olivierlabayle olivierlabayle left a comment

Choose a reason for hiding this comment

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

Thanks Breeshey, in the interest of time I've updated a few file names and some doc. There's two comments to be resolved, please have a look before we merge!

conf/ci_jobs/non_ukbb.config Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
test/non_ukbb.jl Outdated Show resolved Hide resolved
@olivierlabayle
Copy link
Member

@roskamsh I've updated the new test configuration filename to match what it does, if the tests work, I'll merge and release! Thank you again for taking the time! :-)

@olivierlabayle olivierlabayle merged commit 3f96860 into main Dec 4, 2023
8 checks passed
@olivierlabayle olivierlabayle deleted the brh_data_source branch December 4, 2023 19:27
@roskamsh roskamsh self-assigned this Nov 8, 2024
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.

2 participants