Add validator for writing vcf.

[niyarin]

I add VCF4.4 validator for writing.
see also #271

[codecov]

Codecov Report

Merging #277 (def3c5b) into master (212b52c) will increase coverage by 0.01%.
The diff coverage is 89.18%.

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
+ Coverage   88.85%   88.86%   +0.01%     
==========================================
  Files          78       79       +1     
  Lines        6512     6771     +259     
  Branches      458      475      +17     
==========================================
+ Hits         5786     6017     +231     
- Misses        268      279      +11     
- Partials      458      475      +17     

Files	Coverage Δ
src/cljam/io/vcf/util/validator.clj	89.18% <89.18%> (ø)

[alumi]

Thank you for implementing this feature! 👍 I think the basic functionality is good.
I have added a few comments as there seems to be room for improvement.

[alumi]

Thank you for updating. It's getting much better! 👍

[alumi]

Thank you for your continuous efforts in improving this! 🙏 I added some more comments.
Also, some of my previous comments were overlapped by one of them and may have been missed, so please take them into consideration again.

• Add validator for writing vcf. #277 (comment)
• Add validator for writing vcf. #277 (comment)
• Add validator for writing vcf. #277 (comment)
• Add validator for writing vcf. #277 (comment)
• Add validator for writing vcf. #277 (comment)

[athos]

Thanks for drafting this PR and sorry for the terribly late reaction to it!

Personally, I'm thinking that the VCF validator namespace should provide functions like the following as a public API, from the aspects of consistency and extensibility:

• (make-validator <meta-info> <header> <opts>)
• (validate-variant <validator> <variant>)
• (validate-variants <validator> <variants>)

Consistency

invalid-variant? (btw I'd prefer valid-variant?, but anyway) does more than returning a boolean value, so it shouldn’t be defined as a predicate. Rather, I think It would be nice to align its semantics with validate-variants’s and provide a functionality that validates a single variant and reports errors if any.

Extensibility

If we add more validation options in the future, it would be tedious to pass those options to every call to validate-variant(s) (especially when they are used in more than one place). So, I think the validation options should be passed to make-validator, and validate-variant(s) should use the options specified for the validator.

Also, validate-variants could be extended to return a transducer (from the arity without variants), but it already has an optional argument vcf-or-bcf, which makes it a little bit harder to add another arity for the transducer. In this respect, I think vcf-or-bcf should be passed to make-validator as another option, such as :file-type.

We are highly likely to want to add more options for VCF validation in the future, and a validator will be a good place to put together various information including options.

[athos]

JFYI : If we really need to have a function that returns a map containing error information, I’d prefer adding another "verb" for the different semantics. For example:

• (validate-variant <validator> <variant>): Returns a map that contains error information if any. Otherwise returns nil.
• (check-variant <validator> <variant>): Throws an error if the variant is invalid.

[niyarin]

I renamed the api and change make-validator to public.

[athos]

Thanks for the updates!

And sorry that I carelessly suggested using the word "check" without paying attention to the code you already wrote, but currently the word "check" is used interchangeably with the word "validate" (like check-base-records, check-entry-type etc.). We should draw a clear line between how we use those two words.

[athos]

The VCF spec v4.4 says:

Meta-information lines are optional, but if they are present then they must be completely well-formed.
(snip) Note that BCF, the binary counterpart of VCF, requires that all entries are present.
It is recommended to include meta-information lines describing the entries used in the body of the VCF file.

A straightforward interpretation of these lines, I think, is that the validator should not raise an error only because the definition of a format key was not found in the meta info when it validates a VCF file.

[athos]

Ditto.

[athos]

The VCF spec defines integers to be 32-bit and signed, and disallows the values from $-2^{31}$ to $-2^{31}+7$, so the validator should check if the integer value is within the valid range.

[athos]

If seq is called on a non-seqable value, it will throw an error. So, you shouldn't use it to check if a value is a sequence. Calling sequential? is just enough here.

[athos]

The seq here is not necessary.

[athos]

I'd name it validate-samples or something like that. Grammatically speaking, we don’t usually put a plural noun after "each".

[athos]

Users can’t tell which validation error came from which variant, so I think the validation target variant should always be assoced to the error map.

[athos]

Just rewrote the validator implementation.

It should provide basic validation features, but I think it's a bit vulnerable to broken definitions of Info and Genotype fields themselves, so we might want to work on that area as a future work.

[athos]

Approved this, just for form's sake.

[alumi]

@athos Thank you for organizing the API! 🎉
It appears to be consistently structured and more user-friendly than before.
The internal implementation also has a greater sense of uniformity and is well-structured.
I agree with this approach, so I approve the PR

[matsutomo81]

Thank you for creating this PR and I apologize for the significant delay in responding to your review request 🙏
I haven't been able to review all of the code yet, but I commented on a few points I've noticed so far.

[matsutomo81]

I’ve completed the code review.
LGTM! 👍

[athos]

Thank you all for reviewing! I just squashed the commits.