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

Feature/traceback concat #29

Merged
merged 15 commits into from
Feb 21, 2024
Merged

Feature/traceback concat #29

merged 15 commits into from
Feb 21, 2024

Conversation

buehlere
Copy link
Collaborator

  • overhauling pv maf concat to match new file - operation standards
  • tagging for traceback files
  • contribution doc
  • PR template
  • add code owners
  • clean-up

@buehlere buehlere requested a review from rnaidu February 15, 2024 19:34
@@ -0,0 +1,81 @@
# Contributing to Post-Processing
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Eric, I followed these commands when working on the remove_variants_by_annotations.py conversion to a subcommand and was able to understand the instructions clearly. Great job on explaining the steps/resources needed to set up these commands!

Copy link
Contributor

@rnaidu rnaidu left a comment

Choose a reason for hiding this comment

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

Great job! I added a couple of recommendations for adding documentation in the contributing and README docs. The changes are minor and won't affect things downstream, so I am approving the review.


### VarDictJava

The sub-command `pv vardict` allows users to perform post-processing on VarDictJava output. The two supported inputs to `pv vardict` from VarDictJava are `single` and `case-control` vcfs.
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor change, but I'd recommend adding the version of VarDictJava in the README. mostly due to the fact that the docker image created for VarDict that is compatible with M1 is version 1.8.2 (same version used by MSK), but the version used by nf-core is 1.8.3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call. I added the version in the docs/VARDICT.md and added a link in the README.


### Maf

maf concat examples:
Copy link
Contributor

Choose a reason for hiding this comment

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

again very minor, but maybe adding a sentence at the beginning of the README (similar to line 10) with a quick explanation to what the category of 'maf' commands are intended to do in post-processing. When the vcf2maf module has been approved in msk-modules, we could even add a hyperlink to it in the README here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

similar to above I added a link to the README that points to docs/maf

mafa = MAFFile(maf, separator)
mafa = mafa.tag("common_variant")
mafa.to_csv(f"{output_maf}".format(outputFile=output_maf), index=False, sep="\t")
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a review comment but a question-- I noticed you return "0" at the end of your sub-command functions. What is the purpose of returning 0 as compared to a string value indicating the execution of the sub-command has completed or not returning anything at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In python, if I'm writing a file in a function as opposed to returning something, I still like to return a 0 to indicate a successful run of the function. Some languages like C require you return something at the completion of a function. It's not required in python, but something I think is decent style. We can change the return as well.

@buehlere buehlere merged commit ae47489 into develop Feb 21, 2024
6 checks passed
@buehlere buehlere deleted the feature/traceback_concat branch February 21, 2024 15:34
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.

2 participants