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

Add Biscuit #295

Open
wants to merge 31 commits into
base: dev
Choose a base branch
from
Open

Add Biscuit #295

wants to merge 31 commits into from

Conversation

njspix
Copy link
Contributor

@njspix njspix commented Jan 20, 2023

Hello all,

I've put together a draft PR incorporating the Biscuit suite of tools. This is still a work in progress, but I wanted to open the PR and get your feedback.

I did make a few changes to the rest of the workflow. These include version bumps, as well as re-naming and re-organizing a couple of the parameters. I moved a couple of parameters from the 'Bismark options' section to the 'alignment options' section, as these parameters (e.g. 'directional') apply to more than one aligner. I also expanded the 'methylation calling options' section (or created it? I can't remember), which again contains general options (e.g. minimum coverage depth) that apply to multiple tools.

Todos include updating docs (usage, output, changelog, readme), adding CI tests/test profile for the Biscuit workflow, and running various combinations of input and parameters...

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs- [ ] If necessary, also make a PR on the nf-core/methylseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

Thanks much!

@njspix njspix requested a review from ewels January 20, 2023 14:12
@github-actions

This comment was marked as resolved.

@njspix njspix changed the base branch from master to dev January 20, 2023 14:12
@sateeshperi sateeshperi mentioned this pull request May 27, 2023
6 tasks
@sateeshperi
Copy link
Contributor

hi @njspix looks like this might be a nice addition to the pipeline. the branches might have diverged a bit so, might need some care there. How much work do you anticipate is left to push this through ? and if I can be of any help...

@njspix
Copy link
Contributor Author

njspix commented May 30, 2023

Hey @sateeshperi thanks so much for checking in! I would love to get this pushed through as well. However, as I am working on it, I've done quite a bit of reworking, maybe even enough for a major version bump. Things that make sense for two aligners (one set of options for one, another set for the second) start to make less sense for 3 aligners, and there are a lot of common options (e.g. how much coverage to require for a methylation call, whether to do directional alignment, and if so, how, etc) that with some finagling can be generalized to all 3 aligners. Thus, you'll see significant changes to the config and parameters. I think most things are pretty close to working, but unfortunately other responsibilities have taken priority the last few months.

I would estimate it would take me about a weeks' worth of solid effort to put this in a state where I was ready to sign off on every aspect of the workflow, much of that would be testing with various combinations of parameters.

If you would like to take a look through the schema and let me know what you think of the changes I made, that would be most welcome! Module updates etc would be welcome as well...

I hope to get back to working on this part-time in a few weeks, please let me know your thoughts!

@sateeshperi
Copy link
Contributor

sateeshperi commented May 30, 2023

I agree this would be a major version bump on its own and I understand the consolidation of params that this re-work involves and thank you for thinking through it. just want to thrown in there is an issue to add in another aligner too - bismark-minimap2 #257.
I ll help clear out the existing PRs in methylseq first and try to review this branch. you mentioned testing with various combinations of parameters 🔥 plz check out #nf-test channel for using this tool as part of testing framework and I can help with writing tests as well if interested

@sateeshperi sateeshperi added the WIP Work in progress label May 30, 2023
@njspix
Copy link
Contributor Author

njspix commented May 30, 2023

Awesome, thank you so much! Yes, I wouldn't mind adding in support for minimap2 as well - better to do it all at once! :)

@sateeshperi sateeshperi linked an issue Jun 1, 2023 that may be closed by this pull request
@edmundmiller edmundmiller added this to the 3.0.0 milestone Aug 25, 2023
@ewels ewels removed their request for review November 3, 2023 09:15
@njspix njspix requested a review from a team as a code owner January 5, 2024 20:41
Copy link

github-actions bot commented Jan 9, 2024

nf-core lint overall result: Passed ✅

Posted for pipeline commit 6c92ffa

+| ✅ 155 tests passed       |+
#| ❔   4 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.11.1
  • Run at 2024-01-08 22:52:14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Bismark minimap2
3 participants