Updates to CrosscheckFingerprints documentation. #1520

samuelklee · 2020-05-29T18:47:32Z

Changes comments on the fields in the CrosscheckMetric class to Javadoc, in response to https://gatk.broadinstitute.org/hc/en-us/community/posts/360058443032-Interpretting-CrosscheckFingerprints-metrics and subsequent discussion on Slack.

Reviewers can use this PR to add additional documentation and updates, if desired.

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 · 2020-05-29T18:49:33Z

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

@@ -78,23 +78,23 @@
 * and sample for VCF files) and then optionally aggregates it by library, sample or file, to increase power and provide
 * results at the desired resolution. Output is in a "Moltenized" format, one row per comparison. The results will
 * be emitted into a metric file for the class {@link CrosscheckMetric}.
- * In this format the output will include the LOD score and also tumor-aware LOD score which can
- * help assess identity even in the presence of a severe loss of heterozygosity with high purity (which could
+ * In this format, the output will include both the LOD score and the tumor-aware LOD score; the latter can


We may want to expand upon this documentation, I just fixed up a few typos I noticed. I also found it confusing that this text was mostly---but not faithfully---reproduced in the tool summary (and hence did not fix the corresponding typos there).

One other comment while I'm here---"moltenize/d" is a very idiosyncratic term to describe this type of table reshaping. Why not just "molten/melted"?

I have no strong feelings about moltenized....if you think that "molten" is more clear, go for it!

samuelklee · 2020-05-29T18:50:17Z

src/main/java/picard/fingerprint/CrosscheckMetric.java

@@ -59,51 +59,54 @@ public Boolean isMatch() {
        }
    }

+    /** The data type. */


These descriptions should probably be expanded, I just changed them to Javadoc and fixed up a few LEFT vs. RIGHT typos.

yfarjoun

looks good. thanks!

yfarjoun · 2020-06-16T15:49:48Z

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

@@ -78,23 +78,23 @@
 * and sample for VCF files) and then optionally aggregates it by library, sample or file, to increase power and provide
 * results at the desired resolution. Output is in a "Moltenized" format, one row per comparison. The results will
 * be emitted into a metric file for the class {@link CrosscheckMetric}.
- * In this format the output will include the LOD score and also tumor-aware LOD score which can
- * help assess identity even in the presence of a severe loss of heterozygosity with high purity (which could
+ * In this format, the output will include both the LOD score and the tumor-aware LOD score; the latter can


I have no strong feelings about moltenized....if you think that "molten" is more clear, go for it!

samuelklee · 2020-06-16T18:56:00Z

Thanks @yfarjoun! As discussed on Slack with Comms, they’ll take over this PR and use it to address the above comments and make further changes.

derekca · 2020-07-28T19:01:20Z

Hi @yfarjoun, I've made a few clarifying revisions to the intro — CrosscheckFingerprints_Revised

Please take a look and incorporate them as appropriate.

yfarjoun · 2020-08-11T18:13:19Z

why are these changes not integrated into the PR? I'm confused about how you want to work here…..

…

On Tue, Jul 28, 2020 at 3:01 PM Derek Caetano-Anollés < ***@***.***> wrote: Hi @yfarjoun <https://github.com/yfarjoun>, I've made a few clarifying revisions to the intro — CrosscheckFingerprints_Revised <https://github.com/broadinstitute/picard/files/4990547/CrosscheckFingerprints_Revised.txt> Please take a look and incorporate them as appropriate. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1520 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAU6JUUDEAXH3WSR5PEAWQ3R54OA7ANCNFSM4NOIR35A> .

kbergin · 2021-02-22T16:45:21Z

Hi @derekca - would you be able to incorporate your suggested wording into this PR? Going through open Picard PRs and hoping to get them merged in :)

derekca · 2021-02-26T18:51:32Z

@kbergin Just found out about this now... I never got pinged for these responses for some reason.

I either do not have the correct permissions, or am not familiar enough with git, to be able to incorporate these changes to the doc.

kbergin · 2021-02-26T20:49:04Z

@samuelklee would you be able to incorporate the changes that Derek linked above and close out this PR?

gbggrant · 2021-03-17T18:49:17Z

Hi @samuelklee I'm assigning this back to you as it seems to have stalled out.

samuelklee · 2021-03-18T02:03:49Z

Apologies @gbggrant, I should've noted here that User Ed is on this, as was decided on the gatk-userliaison Slack.

I'm probably not actually familiar enough with the tool to make detailed changes or provide a review---I originally opened the PR to help get the ball rolling in response to some discussions I happened to see on Slack and the GATK Forum. I made some obvious changes and fixed some typos, but I think @derekca or someone else more qualified than me will have to bring this home!

jessicaway · 2021-04-14T18:17:56Z

@knoblett Has the User Ed team has had a chance to look at this?

gbggrant · 2021-05-27T17:54:09Z

@samuelklee is there anything holding this PR up from being merged? It's been waiting a while.

samuelklee · 2021-06-01T13:06:23Z

@gbggrant sorry, just seeing your mention after the long weekend. Just wanted to note that this PR was never completed, to my knowledge. As stated a few times above, I made some cursory fixes to get the ball rolling but handed things over to User Ed to finish up. It looks like @derekca came up with some additional changes in #1520 (comment) that were never incorporated.

gbggrant · 2021-09-01T17:26:14Z

@samuelklee can you and @derekca work to get the updated documentation integrated into this PR? I have tried reading the text file provided in the comment above, but it is in a very different format than used in the picard comment and usage statement.

samuelklee · 2021-09-01T17:39:15Z

@gbggrant thanks for making sure this doesn’t fall through the cracks! Unfortunately, my position is still the same as in #1520 (comment) (and I’m also currently on vacation!) User Ed, somebody more familiar with the tool, or anyone else with the bandwidth to review and incorporate the changes in the text file should bring this home.

Again, I only opened this PR as a courtesy with the understanding that others would close it out—no good deed, I suppose! If no one else cares to finish it up, then feel free to close it. Seems a bit of shame to miss a chance to improve documentation, but it’s no skin off my back.

EDIT: see https://broadinstitute.slack.com/archives/CDNPV40AK/p1614274505007300, https://broadinstitute.slack.com/archives/CDNPV40AK/p1590778639131700, https://broadinstitute.slack.com/archives/CDNPV40AK/p1584385024062800, and contemporaneous messages for more context.

samuelklee · 2021-09-13T21:13:24Z

@bhanugandham could I possibly recruit you to reassign or otherwise shepherd this PR?

I'm assuming the edits suggested above by @derekca are good and are an improvement over any typo fixes I might've introduced above. The only nitpick I have upon skimming is that I still think we should still use the standard "molten" to refer to the dataframe melt operation, instead of the rather idiosyncratic "moltenized" (which has <500 hits on Google!)

Can you find someone to incorporate those edits along with the changes I've made to the code? I think you should just be able to add onto this PR, but feel free to copy the branch and open another if needed.

I'm fine with punting on additional changes to the Javadoc, although as commented above I think they could bear expansion.

derekca · 2021-09-15T18:59:06Z

I'm assuming the edits suggested above by @derekca are good and are an improvement over any typo fixes I might've introduced above. The only nitpick I have upon skimming is that I still think we should still use the standard "molten" to refer to the dataframe melt operation, instead of the rather idiosyncratic "moltenized" (which has <500 hits on Google!)

Thanks for the input, I've fixed the comment about molten outputs in the new version.

I've gone ahead and created a separate pull request, 1721, for revising the introduction to this doc.

samuelklee · 2021-09-16T00:33:39Z

Great, thanks @derekca! @gbggrant I've rebased and will wait for tests to pass again before merging. Perhaps then @derekca can resolve any conflicts over in his PR and merge afterwards.

EDIT: Wait, nevermind---I see that @derekca incorporated my changes as well over there. In which case, feel free to close this copy or do what you will with it, whenever you like!

bhanugandham · 2021-09-17T16:10:48Z

@samuelklee and @gbggrant I apologize i didn't see the comments. I was on vacation the last week and a half and just got back today. Looks like @derekca has taken care of this? Please let me know if there is anything else i can help with. Sorry again for the delay!

samuelklee · 2021-09-17T16:29:18Z

No worries, @bhanugandham. Sorry, didn’t realize you were off! I think @derekca is on top of the other PR. Not sure who will review, but I’ll leave that up to you all. Thanks again to both of you!

bhanugandham · 2021-09-17T16:35:46Z

Sounds good! Thanks Sam!

samuelklee commented May 29, 2020

View reviewed changes

yfarjoun approved these changes Jun 16, 2020

View reviewed changes

bhanugandham assigned derekca Jun 17, 2020

gbggrant assigned samuelklee Mar 17, 2021

samuelklee removed their assignment Mar 18, 2021

jessicaway assigned knoblett Apr 14, 2021

jessicaway approved these changes May 27, 2021

View reviewed changes

gbggrant assigned gbggrant and unassigned knoblett and derekca May 27, 2021

gbggrant assigned samuelklee and derekca and unassigned gbggrant and samuelklee Sep 1, 2021

samuelklee removed their assignment Sep 1, 2021

Updates to CrosscheckFingerprints documentation.

68f779a

samuelklee force-pushed the sl_crosscheckfingerprints_docs branch from 23d9d66 to 68f779a Compare September 16, 2021 00:31

gbggrant mentioned this pull request Feb 2, 2022

Update CrosscheckFingerprints summary #1721

Merged

5 tasks

samuelklee closed this Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to CrosscheckFingerprints documentation. #1520

Updates to CrosscheckFingerprints documentation. #1520

samuelklee commented May 29, 2020 •

edited

Loading

samuelklee May 29, 2020

samuelklee May 29, 2020

yfarjoun Jun 16, 2020

samuelklee May 29, 2020

yfarjoun left a comment

yfarjoun Jun 16, 2020

samuelklee commented Jun 16, 2020

derekca commented Jul 28, 2020

yfarjoun commented Aug 11, 2020 via email

kbergin commented Feb 22, 2021

derekca commented Feb 26, 2021

kbergin commented Feb 26, 2021

gbggrant commented Mar 17, 2021

samuelklee commented Mar 18, 2021

jessicaway commented Apr 14, 2021

gbggrant commented May 27, 2021

samuelklee commented Jun 1, 2021 •

edited

Loading

gbggrant commented Sep 1, 2021

samuelklee commented Sep 1, 2021 •

edited

Loading

samuelklee commented Sep 13, 2021 •

edited

Loading

derekca commented Sep 15, 2021

samuelklee commented Sep 16, 2021 •

edited

Loading

bhanugandham commented Sep 17, 2021

samuelklee commented Sep 17, 2021

bhanugandham commented Sep 17, 2021

Updates to CrosscheckFingerprints documentation. #1520

Updates to CrosscheckFingerprints documentation. #1520

Conversation

samuelklee commented May 29, 2020 • edited Loading

Checklist (never delete this)

Content

Review

samuelklee May 29, 2020

Choose a reason for hiding this comment

samuelklee May 29, 2020

Choose a reason for hiding this comment

yfarjoun Jun 16, 2020

Choose a reason for hiding this comment

samuelklee May 29, 2020

Choose a reason for hiding this comment

yfarjoun left a comment

Choose a reason for hiding this comment

yfarjoun Jun 16, 2020

Choose a reason for hiding this comment

samuelklee commented Jun 16, 2020

derekca commented Jul 28, 2020

yfarjoun commented Aug 11, 2020 via email

kbergin commented Feb 22, 2021

derekca commented Feb 26, 2021

kbergin commented Feb 26, 2021

gbggrant commented Mar 17, 2021

samuelklee commented Mar 18, 2021

jessicaway commented Apr 14, 2021

gbggrant commented May 27, 2021

samuelklee commented Jun 1, 2021 • edited Loading

gbggrant commented Sep 1, 2021

samuelklee commented Sep 1, 2021 • edited Loading

samuelklee commented Sep 13, 2021 • edited Loading

derekca commented Sep 15, 2021

samuelklee commented Sep 16, 2021 • edited Loading

bhanugandham commented Sep 17, 2021

samuelklee commented Sep 17, 2021

bhanugandham commented Sep 17, 2021

samuelklee commented May 29, 2020 •

edited

Loading

samuelklee commented Jun 1, 2021 •

edited

Loading

samuelklee commented Sep 1, 2021 •

edited

Loading

samuelklee commented Sep 13, 2021 •

edited

Loading

samuelklee commented Sep 16, 2021 •

edited

Loading