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

Review of PR 301 to Discuss fixes of #164 #168 #169 #375

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

Review of PR 301 to Discuss fixes of #164 #168 #169 #375

wants to merge 7 commits into from

Conversation

lpantano
Copy link

@lpantano lpantano commented Jul 12, 2024 •
edited

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

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit b1fb091

+| ✅ 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-07-15 21:52:11

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoseEspinosa JoseEspinosa Awaiting requested review from JoseEspinosa

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@lpantano @JoseEspinosa