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

Dev #89

Merged
merged 9 commits into from
Jul 13, 2023
Merged

Dev #89

merged 9 commits into from
Jul 13, 2023

Conversation

jianhong
Copy link
Contributor

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/hicar 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
Copy link

github-actions bot commented Jul 10, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 2ad59a0

+| ✅ 153 tests passed       |+
#| ❔   4 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/branch.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting_comment.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-07-13 14:05:06

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Hi @jianhong, are you sure you updated the template using nf-core/tools v2.9? Some of the changes seem outdated.

@jianhong
Copy link
Contributor Author

Hi @jianhong, are you sure you updated the template using nf-core/tools v2.9? Some of the changes seem outdated.

@mirpedrol Thank you for the quick response. I pulled upstream template and merged by

git fetch --all
git merge upstream/nf-core-template-merge-2.9

And then fixed the collision. Did I missed anything?

@mirpedrol
Copy link
Member

That should work. I was looking at it again, and it might be due to some merge conflicts you had, for example you define the nf-validation plugin twice https://github.com/nf-core/hicar/pull/89/files#diff-ce7465a8e67b3b9c95696db1146ca7e6328f075e08a3d64062b87aa8608fc4eaR284 I will leave review comments with the things I find :)

@jianhong
Copy link
Contributor Author

That should work. I was looking at it again, and it might be due to some merge conflicts you had, for example you define the nf-validation plugin twice https://github.com/nf-core/hicar/pull/89/files#diff-ce7465a8e67b3b9c95696db1146ca7e6328f075e08a3d64062b87aa8608fc4eaR284 I will leave review comments with the things I find :)

Thank you for point that out. I removed the duplicated lines.

nextflow.config Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
.csv
.splitCsv ( header:true, sep:',' )
.map { create_fastq_channel(it) }
Channel.fromSamplesheet("input")
Copy link
Member

Choose a reason for hiding this comment

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

This is working fine, so my comment is only a suggestion. You can remove the whole input_check subworkflow by creating the input channel in the main script. If you want to see an example, rnaseq has a PR with these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried but I have trouble in appending the meta.id by map with pseudo-code:

.map{
   meta, fq1, fq2 ->
      meta.id = meta.group + "_REP" + meta.replicate + "_T" + meta.techniquerep // I have trouble here
      [meta, [fq1, fq2]]
}

Copy link
Member

Choose a reason for hiding this comment

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

This is modifying the meta map in place, which can lead to errors in the pipeline. An alternative is using the operators + or - which create a copy of the object instead.
meta - meta.subMap('id') + [id: meta.group + "_REP" + meta.replicate + "_T" + meta.techniquerep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the input_check.nf. Could you please take a second look?

subworkflows/local/input_check.nf Show resolved Hide resolved
workflows/hicar.nf Outdated Show resolved Hide resolved
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Some last comments, after these changes looks good to me :)

Comment on lines +17 to +18
if (!params.gtf && !params.gff) {
Nextflow.error("No GTF or GFF3 annotation specified! The pipeline requires at least one of these files.")
Copy link
Member

Choose a reason for hiding this comment

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

Do you have params.gtf and params.gff? I can't see them in nextflow.config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are appended the main.nf.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, then looks good :)

Comment on lines 183 to 184
meta.group = meta.group.toString().replaceAll("\\.", "_")
meta - meta.subMap(['id', 'single_end']) + [id: meta.group + "_REP" + meta.replicate + "_T" + meta.techniquereplicate, single_end: false]
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
meta.group = meta.group.toString().replaceAll("\\.", "_")
meta - meta.subMap(['id', 'single_end']) + [id: meta.group + "_REP" + meta.replicate + "_T" + meta.techniquereplicate, single_end: false]
meta.group = meta.group.toString().replaceAll("\\.", "_")
meta - meta.subMap(['id', 'single_end', 'group']) + [id: meta.group + "_REP" + meta.replicate + "_T" + meta.techniquereplicate, single_end: false, 'group': meta.group.toString().replaceAll("\\.", "_")]

@jianhong jianhong merged commit 41312de into nf-core:dev Jul 13, 2023
32 checks passed
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.

3 participants