Add an option to allow records with malformed Number=A fields. #406
Conversation
Pull Request Test Coverage Report for Build 1428
💛 - Coveralls |
allieychen
left a comment
allieychen
left a comment
There was a problem hiding this comment.
It looks awesome! I only have some minor comments.
| minimal_match=False, # type: bool | ||
| infer_annotation_types=False, # type: bool | ||
| counter_factory=None # type: metrics_util.CounterFactoryInterface | ||
| counter_factory=None, # type: metrics_util.CounterFactoryInterface, |
There was a problem hiding this comment.
nit: remove , at the end.
There was a problem hiding this comment.
nit: I prefer to switch the order of counter_factory and allow_alternate_allele_info_mismatch, so that all bool flags can stay together to improve the readability.
There was a problem hiding this comment.
Great suggestion! I actually moved it all the way up to be close to split_alternate_allele_info_fields as they're related.
gcp_variant_transforms/vcf_to_bq.py
Outdated
| known_args.minimal_vep_alt_matching, | ||
| known_args.infer_annotation_types, | ||
| counter_factory) | ||
| counter_factory, |
There was a problem hiding this comment.
It may not related with this PR, but is there any reason why not create the counter inside ProcessedVariantFactory?
There was a problem hiding this comment.
Good question. The main reason is to share the factory across all classes (currently only one classes uses counters, but we have some open issues to add counters to other classes too) and also that we want to override it in tests to use the NoOpCounterFactory (or any other custom metric factory).
| self._variant_counter = cfactory.create_counter( | ||
| _CounterEnum.VARIANT.value) | ||
| self._alternate_allele_info_mismatche_counter = cfactory.create_counter( | ||
| _CounterEnum.ALTERNATE_ALLELE_INFO_MISMATCH) |
There was a problem hiding this comment.
nit: s/ALTERNATE_ALLELE_INFO_MISMATCH/ALTERNATE_ALLELE_INFO_MISMATCH.value, as the above counter _variant_counter used VARIANT.value
There was a problem hiding this comment.
Nice catch! Done.
arostamianfar
left a comment
arostamianfar
left a comment
There was a problem hiding this comment.
Thanks for the detailed review!!
| self._variant_counter = cfactory.create_counter( | ||
| _CounterEnum.VARIANT.value) | ||
| self._alternate_allele_info_mismatche_counter = cfactory.create_counter( | ||
| _CounterEnum.ALTERNATE_ALLELE_INFO_MISMATCH) |
There was a problem hiding this comment.
Nice catch! Done.
| minimal_match=False, # type: bool | ||
| infer_annotation_types=False, # type: bool | ||
| counter_factory=None # type: metrics_util.CounterFactoryInterface | ||
| counter_factory=None, # type: metrics_util.CounterFactoryInterface, |
There was a problem hiding this comment.
Great suggestion! I actually moved it all the way up to be close to split_alternate_allele_info_fields as they're related.
gcp_variant_transforms/vcf_to_bq.py
Outdated
| known_args.minimal_vep_alt_matching, | ||
| known_args.infer_annotation_types, | ||
| counter_factory) | ||
| counter_factory, |
There was a problem hiding this comment.
Good question. The main reason is to share the factory across all classes (currently only one classes uses counters, but we have some open issues to add counters to other classes too) and also that we want to override it in tests to use the NoOpCounterFactory (or any other custom metric factory).
3 participants
This option gracefully handles both excess and insufficient values. For now, the flag is set via
allow_malformed_recordsin order to avoid having too many flags. We can consider adding a separate flag if users want to control this behavior separate from other malformed conditions.Tested:
unit + integration tests
Fixes #129