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.

Sign up for GitHub

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

Jump to bottom

add column for std nomenclature #6043

Merged
merged 1 commit into from
Jul 29, 2019
Merged

add column for std nomenclature #6043

merged 1 commit into from
Jul 29, 2019

Conversation

tedsharpe
Copy link
Contributor

No description provided.

@tedsharpe tedsharpe requested a review from SHuang-Broad July 16, 2019 19:38
@codecov
Copy link

codecov bot commented Jul 16, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@1f31a80). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             master     #6043   +/-   ##
==========================================
  Coverage          ?   44.078%           
  Complexity        ?     20088           
==========================================
  Files             ?      2011           
  Lines             ?    151051           
  Branches          ?     16160           
==========================================
  Hits              ?     66580           
  Misses            ?     79486           
  Partials          ?      4985
Impacted Files Coverage Δ Complexity Δ
...er/tools/AnalyzeSaturationMutagenesisUnitTest.java 0.794% <0%> (ø) 2 <0> (?)
...hellbender/tools/AnalyzeSaturationMutagenesis.java 4.601% <0%> (ø) 0 <0> (?)

@tedsharpe tedsharpe force-pushed the tws_analyzeSatMut_tweaks branch from 9aa34a7 to ad7beff Compare July 23, 2019 19:48
@tedsharpe
Copy link
Contributor Author

fixed unit test

SHuang-Broad
SHuang-Broad reviewed Jul 24, 2019
View reviewed changes
Copy link
Contributor

@SHuang-Broad SHuang-Broad left a comment

Choose a reason for hiding this comment

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

I have some confusions which most likely is due to my lack of knowledge.

Also, for the failed unit test earlier, do you think a small test on that (newly added "filter") is appropriate?

src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
}
}

private static boolean isSynonymous( final CodonVariation variation, final int[] refCodonValues ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit confused (totally possible due to my lack of knowledge of biology): synonymous to what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SNVs in coding regions are either synonymous (they don't change the amino acid incorporated into the protein), missense (the base change changes the AA), or nonsense (the base change creates a stop code that truncates the protein).
I'll add a comment that includes the words "silent mutation".

src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
public CodonVariationGroup( final int[] refCodonValues, final CodonVariation codonVariation ) {
this.refCodonValues = refCodonValues;
altCalls = new StringBuilder();
switch ( codonVariation.getVariationType() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For these count values, I think it is easier to understand if they are assigned value 1 in this ctor, instead of adding 1, unless I'm misunderstanding something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. That would be clearer.

src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java Outdated
public boolean isEmpty() { return subCount + insCount + delCount == 0; }

// translate the group into standard nomenclature
@Override public String toString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

may I suggest a different name for this method?
tostring() to me is almost always for debugging.
This method is clearly doing very important tasks.

What about toStandardNomanclature()?

tedsharpe
tedsharpe commented Jul 25, 2019
View reviewed changes
Copy link
Contributor Author

@tedsharpe tedsharpe 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 your review, Steve. I believe your comments have been addressed.

src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
}
}

private static boolean isSynonymous( final CodonVariation variation, final int[] refCodonValues ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SNVs in coding regions are either synonymous (they don't change the amino acid incorporated into the protein), missense (the base change changes the AA), or nonsense (the base change creates a stop code that truncates the protein).
I'll add a comment that includes the words "silent mutation".

src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java
public CodonVariationGroup( final int[] refCodonValues, final CodonVariation codonVariation ) {
this.refCodonValues = refCodonValues;
altCalls = new StringBuilder();
switch ( codonVariation.getVariationType() ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. That would be clearer.

src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/AnalyzeSaturationMutagenesis.java Show resolved Hide resolved
@tedsharpe tedsharpe force-pushed the tws_analyzeSatMut_tweaks branch from c78d21e to 9d8b445 Compare July 29, 2019 14:49
@tedsharpe tedsharpe merged commit d01315d into master Jul 29, 2019
@tedsharpe tedsharpe deleted the tws_analyzeSatMut_tweaks branch July 29, 2019 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SHuang-Broad SHuang-Broad SHuang-Broad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tedsharpe @SHuang-Broad