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 #24

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Dev #24

wants to merge 33 commits into from

Conversation

tanyasarkjain
Copy link
Collaborator

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/pacvar branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

CHANGELOG.md Outdated
Comment on lines 10 to 18
## v1.0.1 - Sardine [02/26/2025]

### `Added`

### `Fixed`

- Changed files produced downstream from PBSV to have an output file name containing 'pbsv' to indicate origin of the files
- Tweaks to the channels passed into HiPhase - specifically ensure that the inputted VCF and BAM channel are ordered in the same way (according to their shared meta).

Copy link
Member

Choose a reason for hiding this comment

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

New releases should go above the old release in the changelog

Also other convensions:

Put a link to the PR at the beginning of each entry and tag the person who made teh change, eg..

Suggested change
## v1.0.1 - Sardine [02/26/2025]
### `Added`
### `Fixed`
- Changed files produced downstream from PBSV to have an output file name containing 'pbsv' to indicate origin of the files
- Tweaks to the channels passed into HiPhase - specifically ensure that the inputted VCF and BAM channel are ordered in the same way (according to their shared meta).
## v1.0.1 - Sardine [02/26/2025]
### `Added`
### `Fixed`
- [#19](https://github.com/nf-core/pacvar/pull/19) Changed files produced downstream from PBSV to have an output file name containing 'pbsv' to indicate origin of the files (by
@tanyasarkjain)
- Tweaks to the channels passed into HiPhase - specifically ensure that the inputted VCF and BAM channel are ordered in the same way (according to their shared meta).

Comment on lines 123 to 128
bam_bai_vcf_snp_ch
.multiMap { meta, bam, bai, vcf, tbi ->
bam_bai: [meta, bam, bai]
vcf_tbi: [meta, vcf, tbi]
}
.set { orderd_bam_bai_vcf_tbi_snp }
Copy link
Member

Choose a reason for hiding this comment

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

As it look elsewhere you're using = for channel assignment I would do that everywhere for consistency instead of .set

Suggested change
bam_bai_vcf_snp_ch
.multiMap { meta, bam, bai, vcf, tbi ->
bam_bai: [meta, bam, bai]
vcf_tbi: [meta, vcf, tbi]
}
.set { orderd_bam_bai_vcf_tbi_snp }
orderd_bam_bai_vcf_tbi_snp = bam_bai_vcf_snp_ch
.multiMap { meta, bam, bai, vcf, tbi ->
bam_bai: [meta, bam, bai]
vcf_tbi: [meta, vcf, tbi]
}

bam_bai: [meta, bam, bai]
vcf_tbi: [meta, vcf, tbi]
}
.set { orderd_bam_bai_vcf_tbi_sv }
Copy link
Member

Choose a reason for hiding this comment

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

See above

docs/output.md Outdated
Comment on lines 107 to 112
- `<basename>.phased.bam`: Haplotagged BAM - outputted from phasing based on SNV
- `<basename>.pbsv.phased.bam`: Haplotagged BAM - outputted from phasing based on SV
- `<basename>.phased.vcf`: The phased Variant File (SNV) (Zipped)
- `<basename>.pbsv.phased.vcf`: The phased Variant File (SV) (Zipped)
- `<basename>.stats.csv`: This CSV/TSV file contains information about the the phase blocks that were output by HiPhase (SNV)
- `<basename>.pbsv.stats.csv`: This CSV/TSV file contains information about the the phase blocks that were output by HiPhase (SV)
Copy link

Choose a reason for hiding this comment

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

Now that I see this here: Would it make sense to set .snv. and .sv. to understand the different outputs from the filenames directly?

@tanyasarkjain
Copy link
Collaborator Author

I have resolved these comments in #25 which should contain all the changes suggested, if you could take a look. Further I see there is a conflict with nextflow.config, but due to permissions im unable to look and resolved the conflict myself - how should I go about this? Thanks!

@jfy133
Copy link
Member

jfy133 commented Feb 28, 2025

@tanyasarkjain I've upgraded your repo permissions now (with great pwoer comes great responsbility!) Can you do it now?

Co-authored-by: James A. Fellows Yates <[email protected]>
@tanyasarkjain
Copy link
Collaborator Author

tanyasarkjain commented Feb 28, 2025

@jfy133 Hi James! I still seem to be seeing the "dev is a protected branch" unfortunately and unable to click the "resolve conflicts"

requested changes for 1.0.1 release
@jfy133
Copy link
Member

jfy133 commented Feb 28, 2025

@tanyasarkjain you need to do a manual merge, check the bottom of the box:

image

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.

5 participants