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.

Sign up for GitHub

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

Jump to bottom

Implement "shift_reads" parameter #375

Merged
merged 16 commits into from
Sep 26, 2024
Merged

Implement "shift_reads" parameter #375

merged 16 commits into from
Sep 26, 2024

Conversation

lpantano
Copy link

@lpantano lpantano commented Jul 12, 2024 •
edited
Loading

Effort to review previous PR that discussed important changes in the pipeline from @ktrns.

ONLY adapting the work in #164 (partly issue #91) – A new parameter ‘--shift_reads’.

Originally, this was the PR:
Fixed issue Genome annotation (gene names) missing in IGV session file #168 – The genome is again written out so that the IGV session file can be opened and used. This bug came with atacseq v2.0
Fixed issue Peak calling parameters might not be ideal #169 – Genome coverage tracks were generated & peaks were called based on fragments rather than reads, which is not correct for ATAC-seq analysis. This is fixed.
Implemented issue #164 (partly issue #91) – A new parameter ‘--shift_reads’ adjusts the coordinates of the reads using deeptools/alignmentsieve. At the moment this only works for paired-end data and throws an error if applied to single-end data.

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/atacseq 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).

@github-actions GitHub Actions

This comment was marked as off-topic.

Sign in to view
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 12, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 347d98e

+| ✅ 221 tests passed       |+
#| ❔   2 tests were ignored |#
!| ❗   5 tests had warnings |!

❗ Test warnings:

  • readme - README contains the placeholder zenodo.XXXXXXX. This should be replaced with the zenodo doi (after the first release).
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

  • nextflow_config - Config default ignored: params.bamtools_filter_pe_config
  • nextflow_config - Config default ignored: params.bamtools_filter_se_config

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-09-25 21:06:22

@lpantano lpantano changed the base branch from master to dev July 13, 2024 13:01
@lpantano lpantano marked this pull request as ready for review July 15, 2024 21:50
@lpantano
Copy link
Author

@JoseEspinosa , do you think you can take a look, happy to do anything else to make it easier to review, thanks!

@JoseEspinosa
Copy link
Member

Hi, I am currently on holiday, I will try to take a look but I am not sure when I will be able to.
I was working on preparing a release for chipseq that is almost ready and my idea was to prepare it also for atacseq but did not have enough time.
As there are many changes in the current dev branch and the change you are suggesting is quite a change for the pipeline I am tempted to instead of merging it to dev directly, first make a release of the pipeline with the current bug fixes and some other urgent fixes (version 2.2.0) and then a follow-up release with what you are proposing after doing some testing (version 2.3.0).
If you agree, then maybe I can create a branch to include your changes in the main repository footprinting maybe? and we will merge this PR to this new branch instead of dev, what do you think @lpantano ?

@lpantano
Copy link
Author

I am ok with that, my changes are not that big thought, I only applied the shift and it is turn off by default. I didn't end up changing anything else, because I saw that were major options like the change in the alignments. Take a look, and let me know, I am ok either way, as far as it is the path to get it for other people. This is something very standard done for ATACseq in many other pipelines so I think many people are interested. Thanks!

@lpantano lpantano changed the title Review of PR 301 to Discuss fixes of #164 #168 #169 Review of PR 301 to Discuss fixes of #164 Sep 17, 2024
apeltzer
apeltzer approved these changes Sep 19, 2024
View reviewed changes
Copy link
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Some minor things, but overall ok. Would push the module to nf-core/modules and import from there if possible, looks quite good to me to do so ...

CHANGELOG.md Outdated Show resolved Hide resolved
modules/local/deeptools_alignmentsieve.nf
Copy link
Member

Choose a reason for hiding this comment

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

Would rather push this to nf-core/modules - there is already other recipes for deeptools tools --> another one being added should be fairly easy to do :)

Copy link
Member

Choose a reason for hiding this comment

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

And it looks good :)

Copy link
Author

Choose a reason for hiding this comment

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

yes, I will try to do that, it is true is already there so it should be even easier. thanks!

@lpantano @apeltzer
Update CHANGELOG.md
58be205 
Co-authored-by: Alexander Peltzer <[email protected]>
@JoseEspinosa JoseEspinosa mentioned this pull request Sep 25, 2024
Closed
4 tasks
@lpantano lpantano mentioned this pull request Sep 25, 2024
Merged
17 tasks
JoseEspinosa
JoseEspinosa requested changes Sep 25, 2024
View reviewed changes
Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Some small suggestions, mostly formatting issues. I realised that for chromap we are applying the shift always, I will open an issue and by default will run chromap without shifting if the parameter shift_reads is not set.

workflows/atacseq.nf Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
modules/local/deeptools_alignmentsieve.nf
@@ -0,0 +1,36 @@
process DEEPTOOLS_ALIGNMENTSIEVE {
Copy link
Member

Choose a reason for hiding this comment

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

I agree @apeltzer and opened an issue in the modules repo to create the module there, by the moment we can leave it local in dev

subworkflows/local/bam_shift_reads.nf Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
workflows/atacseq.nf Outdated Show resolved Hide resolved
workflows/atacseq.nf Outdated Show resolved Hide resolved
workflows/atacseq.nf
Comment on lines +382 to +383
// MERGED_LIBRARY_FILTER_BAM.out.bam.join(MERGED_LIBRARY_FILTER_BAM.out.flagstat, by: [0]),
// ch_chrom_sizes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// MERGED_LIBRARY_FILTER_BAM.out.bam.join(MERGED_LIBRARY_FILTER_BAM.out.flagstat, by: [0]),
// ch_chrom_sizes

workflows/atacseq.nf Outdated Show resolved Hide resolved
workflows/atacseq.nf Outdated Show resolved Hide resolved
lpantano and others added 8 commits September 25, 2024 16:53
@lpantano @JoseEspinosa
Update nextflow_schema.json
c57f303 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@lpantano @JoseEspinosa
Update subworkflows/local/bam_shift_reads.nf
25055dd 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@lpantano @JoseEspinosa
Update nextflow_schema.json
c32f71e 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@lpantano @JoseEspinosa
Update workflows/atacseq.nf
fda14bd 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@lpantano @JoseEspinosa
Update workflows/atacseq.nf
50afafb 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@lpantano @JoseEspinosa
Update workflows/atacseq.nf
f3ff187 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@lpantano @JoseEspinosa
Update format include workflows/atacseq.nf
f11e231 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
@lpantano @JoseEspinosa
Update channel format workflows/atacseq.nf
347d98e 
Co-authored-by: Jose Espinosa-Carrasco <[email protected]>
JoseEspinosa
JoseEspinosa approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

Thanks! Awesome job 🚀

@JoseEspinosa JoseEspinosa merged commit ea927b5 into dev Sep 26, 2024
11 checks passed
@JoseEspinosa JoseEspinosa changed the title Review of PR 301 to Discuss fixes of #164 Implement "shift_reads" parameter Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoseEspinosa JoseEspinosa JoseEspinosa approved these changes

@apeltzer apeltzer apeltzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lpantano @JoseEspinosa @apeltzer