-
Notifications
You must be signed in to change notification settings - Fork 592
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
Added in a VcfFuncotationFactory. #4593
Conversation
Fixes #4375 Need to release new copy of data sources along with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff.... back to you @jonn-smith
@@ -812,7 +813,7 @@ private String createProgramGroupID(final SAMFileHeader header) { | |||
final String name, | |||
final Class<? extends Feature> featureType) { | |||
|
|||
final FeatureInput<? extends Feature> featureInput = new FeatureInput<>(name + ":" + filePath); | |||
final FeatureInput<? extends Feature> featureInput = new FeatureInput<>(name + FEATURE_ARGUMENT_TAG_DELIMITER + filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor... Can you change this to FeatureInput.FEATURE_ARGUMENT_TAG_DELIMITER
and change the import statement above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
* @param gencodeFuncotations {@link List} of {@link GencodeFuncotation} that have already been created for the given {@code variant}/{@code referenceContext}/{@code featureContext}. | ||
* @return {@link List} of {@link Funcotation} given the {@code variant}, {@code referenceContext}, and {@code featureContext}. This should never be empty. | ||
*/ | ||
public List<Funcotation> createFuncotations(final VariantContext variant, final ReferenceContext referenceContext, final Map<String, List<Feature>> featureSourceMap, final List<GencodeFuncotation> gencodeFuncotations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you combine this method with the one above to leverage shared code? If a data source needs gencode info, but createFuncotation(...)
is called without Gencode, will an exception be thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! Yeah - I forgot to combine them.
Yeah - I'll add that in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - the exception gets thrown in the concrete classes implementations of createFuncotations
VCF("vcf") { | ||
@Override | ||
public void assertConfigFilePropertiesAreValid(final Properties configFileProperties, final Path configFilePath) { | ||
// There is no special check required for vcf files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a sentence of "why not" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
@@ -191,13 +197,13 @@ public String getName() { | |||
* This method should never be called on a {@link CosmicFuncotationFactory} - knowledge of the applied | |||
* {@link GencodeFuncotation}s is required to create a {@link Funcotation} from here. | |||
*/ | |||
public List<Funcotation> createFuncotations(final VariantContext variant, final ReferenceContext referenceContext, final List<Feature> featureList) { | |||
protected List<Funcotation> createFuncotationsOnVariant(final VariantContext variant, final ReferenceContext referenceContext, final List<Feature> featureList) { | |||
// TODO: this should be allowed, but with a warning to the user that only annotations with good Genome Positions can be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet.
Added as issue #4594
OK @LeeTL1220 - first round comments addressed. |
@jonn-smith OK by me to merge then. |
Codecov Report
@@ Coverage Diff @@
## master #4593 +/- ##
===============================================
- Coverage 79.839% 79.837% -0.002%
- Complexity 16958 16995 +37
===============================================
Files 1062 1063 +1
Lines 61680 61802 +122
Branches 9983 10008 +25
===============================================
+ Hits 49245 49341 +96
- Misses 8540 8556 +16
- Partials 3895 3905 +10
|
* Added in a VcfFuncotationFactory. Fixes broadinstitute#4357 (in conjunction with a new data sources release)
Fixes #4357
Need to release new copy of data sources along with this.