methylseq icon indicating copy to clipboard operation
methylseq copied to clipboard
verified publisher icon nf-core

Add Biscuit

Open njspix opened this issue 2 years ago • 6 comments

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

  • [x] This comment contains a description of changes (with reason).
  • [ ] If you've fixed a bug or added code that should be tested, add tests!
  • [x] 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.
  • [x] Make sure your code lints (nf-core lint).
  • [x] 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 avatar Jan 20 '23 14:01 njspix

This PR is against the master branch :x:

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @njspix,

It looks like this pull-request is has been made against the njspix/methylseq master branch. The master branch on nf-core repositories should always contain code from the latest release. Because of this, PRs to master are only allowed if they come from the njspix/methylseq dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page. Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

github-actions[bot] avatar Jan 20 '23 14:01 github-actions[bot]

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...

sateeshperi avatar May 30 '23 15:05 sateeshperi

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!

njspix avatar May 30 '23 18:05 njspix

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 :fire: 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 avatar May 30 '23 19:05 sateeshperi

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

njspix avatar May 30 '23 20:05 njspix

nf-core lint overall result: Passed :white_check_mark:

Posted for pipeline commit 6c92ffa

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

:grey_question: Tests ignored:

:white_check_mark: Tests passed:

Run details

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

github-actions[bot] avatar Jan 09 '24 14:01 github-actions[bot]