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

Update CrosscheckFingerprints summary #1721

Merged
merged 8 commits into from
Feb 14, 2023

Conversation

derekca
Copy link
Contributor

@derekca derekca commented Sep 15, 2021

Description

I've revised the introduction to the CrosscheckFingerprints doc for improved readability, which was a common criticism of this particular doc at the time of the original drafting of this revision.

The reference to "moltenized" output has been changed to "molten" for the sake of standardization, as per samuelklee's suggestion on the previous draft.


Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

samuelklee and others added 4 commits May 29, 2020 14:44
I've revised the introduction to the CrosscheckFingerprints doc for improved readability. The previous reference to "moltenized" output has been changed to "molten" for the sake of standardization.
Copy link
Contributor

@yfarjoun yfarjoun left a comment

Choose a reason for hiding this comment

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

Thanks for the refresh.

"logged. As long as there is at least one comparison where both sides have observations at fingerprinting sites, the tool will " +
"return zero. However, if all comparisons have at least one side with no observations at fingerprinting sites, an error will be " +
"logged and the tool will return EXIT_CODE_WHEN_NO_VALID_CHECKS." +
"Note that, as long as there is at least one comparison in which both files have observations at fingerprinting sites, the tool will return a ‘zero’. However, an error will be logged and the tool will return EXIT_CODE_WHEN_NO_VALID_CHECKS if all comparisons have at least one side without observations at a fingerprinting site (ie. all LOD scores are zero). \n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

here too, as in other places, I think that a long line of text is harder to read in code...please revert this back to multiple lines....

@gbggrant gbggrant assigned samuelklee and derekca and unassigned samuelklee Sep 29, 2021
@gbggrant
Copy link
Contributor

gbggrant commented Feb 2, 2022

@derekca is this PR ready to merge? It looks like it is holding up #1520 ?

@samuelklee
Copy link
Contributor

Thanks again for keeping on top of this, @gbggrant. Just to clarify, with the way things shook out, it looks like #1520 essentially operated as a draft for this PR. I'm going to assume that all desired changes from that draft were incorporated here and will go ahead and close it.

@kachulis
Copy link
Contributor

@derekca can you update this with Yossi's comments?

derekca and others added 2 commits November 3, 2022 17:35
Apologies for removing the line breaks, I wasn't aware I was affecting the readability. My focus was on the content. Hopefully I've addressed everything.
@droazen droazen changed the base branch from sl_crosscheckfingerprints_docs to master November 15, 2022 18:39
@lbergelson lbergelson assigned kachulis and unassigned derekca Feb 14, 2023
@kachulis kachulis merged commit 58fdb7b into master Feb 14, 2023
@kachulis kachulis deleted the dc_update_CrosscheckFingerprints_summary_200915 branch February 14, 2023 20:02
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.

6 participants