feat: Support reading Picard-style metrics files with comments above the header

[msto]

It would be helpful if Metric.read could parse metrics files with comments above the header, such as those produced by Picard. I've updated Metric.read to use the read_header() method introduced in #124 , and also simplified the typing on io.to_reader() and io.to_writer() (per Clint's suggestion)

---

note I want to keep this separate from the MetricWriter feature branch, but it's dependent on the Metric._read_header method introduced in that branch.

I am waiting to merge until #123 is merged.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.07%. Comparing base (032388d) to head (6b094cc).

❗ Current head 6b094cc differs from pull request most recent head c8a083c

Please upload reports for the commit c8a083c to get more accurate results.

Additional details and impacted files

@@                         Coverage Diff                         @@
##           ms_metric-writer-feature-branch     #103      +/-   ##
===================================================================
+ Coverage                            88.56%   94.07%   +5.50%     
===================================================================
  Files                                   16       33      +17     
  Lines                                 1775     3441    +1666     
  Branches                               378      706     +328     
===================================================================
+ Hits                                  1572     3237    +1665     
- Misses                                 134      135       +1     
  Partials                                69       69              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TedBrookings]

LGTM. I think you should wait to merge until Nils signs off. My general sense is that Metric at least originated as an internal tool for Fulcrum code, not a general-purpose TSV <-> data utility. It would then read and write in a way that Fulcrum approved of. Header comments and CSV instead of TSV were deliberately excluded. I personally don't see the harm in relaxing the allowed inputs while retaining strict outputs.

[msto]

That's fair.

I would be open to refactoring so .read() remains specific to Metric-formatted files. In that case I'd suggest providing a .read_picard() method or similar, which would filter comment lines before calling .read() under the hood.

I do think it would be generally valuable to have built-in support for reading Picard metrics into a Metric, and it would be very specifically helpful for one of my clients.

[msto]

@nh13 thoughts?

[nh13]

@msto I'd like to review this one before it gets merged

[clintval]

I'm indifferent on the PR because for Picard files I use this library:

• https://pypi.org/project/crimson/

It's not typed but it's been good enough for me since it doesn't force a co-install of things like pysam.

[tfenne]

Some reasonably minor suggestions.

[nh13]

Minor comments from me, but a few others to fixup from others.

[nh13]

LGTM, take it or leave the additional comments/suggestions.

[nh13]

Is this backwards incompatible? Should we bump the minor version?

[msto]

Good question, it isn't.

One possible solution for backwards compatibility - I could update Metric._read_header to take an Optional comment prefix, and return the first line if the prefix is None (i.e. no skipping of prefixed or blank lines)

Is that something you'd like to see, or should I leave as is and bump the version?

[nh13]

I'd prefer it to the Optional[str] = None, but I don't have that strong of a feeling.

I'd also ask if comment_prefix is the name we want to go with for this parameter. @tfenne is a good judge of naming.

[tfenne]

I might suggest comment_symbol?

Also i agree with @nh13 that ideally the method gets expanded to include both:

comment_symbol: Optional[str] = None,
skip_blank_lines: bool = False

I might even argue for skip_blank_lines to even be a regex that defines lines to skip ... so e.g. I could use skip_lines_matching = "$\s*^" to skip stupid lines that are all tabs/spaces but no content!

[msto]

note I want to keep this separate from the MetricWriter feature branch, but it's dependent on the Metric._read_header method introduced in that branch.

I am waiting to merge until #123 is merged.