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

Changed referenceBases to silently fail and emit a single warning if there is no reference #3299

Merged
merged 1 commit into from
Aug 21, 2017

Conversation

jamesemery
Copy link
Collaborator

No description provided.

@codecov-io
Copy link

codecov-io commented Jul 18, 2017

Codecov Report

Merging #3299 into master will increase coverage by 0.008%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #3299       +/-   ##
===============================================
+ Coverage     80.366%   80.374%   +0.008%     
- Complexity     17666     17673        +7     
===============================================
  Files           1178      1179        +1     
  Lines          63864     63880       +16     
  Branches        9930      9930               
===============================================
+ Hits           51325     51343       +18     
+ Misses          8584      8582        -2     
  Partials        3955      3955
Impacted Files Coverage Δ Complexity Δ
...bender/tools/walkers/annotator/ReferenceBases.java 100% <100%> (ø) 5 <2> (+1) ⬆️
...titute/hellbender/utils/logging/OneShotLogger.java 100% <100%> (ø) 3 <3> (?)
...nstitute/hellbender/engine/ShardBoundaryShard.java 100% <0%> (ø) 7% <0%> (+3%) ⬆️
...e/hellbender/engine/spark/SparkContextFactory.java 73.973% <0%> (+2.74%) 11% <0%> (ø) ⬇️

@droazen droazen self-assigned this Jul 18, 2017
@droazen droazen self-requested a review July 18, 2017 22:39
@Override
public List<String> getKeyNames() { return Collections.singletonList(REFERENCE_BASES_KEY); }

@Override
public Map<String, Object> annotate(final ReferenceContext ref,
final VariantContext vc,
final ReadLikelihoods<Allele> likelihoods) {

if (ref==null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I though that #2799 suggest to throw an UserException, which actually makes sense if the user provides an annotation that couldn't be apply because the requirements are not met...

Copy link
Contributor

Choose a reason for hiding this comment

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

The warning seems good for now, and consistent with existing behavior in the annotation classes -- we can open a separate ticket to discuss whether there should be a mode in which GATK throws in these cases.

…exsist and added a one time logger functionality
Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

One minor comment, then good to merge 👍

* A logger wrapper class which only outputs the first warning provided to it
*/
public class OneShotLogger {
Logger logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark as @VisibleForTesting

@Override
public List<String> getKeyNames() { return Collections.singletonList(REFERENCE_BASES_KEY); }

@Override
public Map<String, Object> annotate(final ReferenceContext ref,
final VariantContext vc,
final ReadLikelihoods<Allele> likelihoods) {

if (ref==null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The warning seems good for now, and consistent with existing behavior in the annotation classes -- we can open a separate ticket to discuss whether there should be a mode in which GATK throws in these cases.

@droazen droazen removed their assignment Aug 21, 2017
@jamesemery jamesemery merged commit c9839ce into master Aug 21, 2017
cwhelan pushed a commit that referenced this pull request Aug 29, 2017
…exsist and added a one time logger functionality (#3299)

Fixes #2799
jonn-smith pushed a commit that referenced this pull request Aug 29, 2017
…exsist and added a one time logger functionality (#3299)

Fixes #2799
@davidbenjamin davidbenjamin deleted the je_ReferenceBases_2799 branch February 24, 2021 15:05
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.

4 participants