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

Adding the option to specify the sort order of an output SAM file produced by SamBuilder. #25

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

NatPRoach
Copy link
Contributor

@NatPRoach NatPRoach commented Dec 27, 2022

The support for TemplateCoordinate sorted records is coming in the first wave of wrapped pysam samtools dispatch calls.

This should address #20.

This is also more or less identical to an addition made to samwell in a PR merged presumably after the sambuilder module code was pulled over from samwell to here.

Copy link
Member

@nh13 nh13 left a comment

Choose a reason for hiding this comment

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

minor comments

fgpyo/sam/builder.py Outdated Show resolved Hide resolved
fgpyo/sam/builder.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2022

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (273c81c) 92.91% compared to head (ae5b948) 93.01%.

Files Patch % Lines
fgpyo/sam/builder.py 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   92.91%   93.01%   +0.09%     
==========================================
  Files          30       30              
  Lines        3079     3119      +40     
  Branches      572      579       +7     
==========================================
+ Hits         2861     2901      +40     
  Misses        145      145              
  Partials       73       73              

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

@NatPRoach NatPRoach requested a review from nh13 December 29, 2022 21:48
fgpyo/sam/builder.py Outdated Show resolved Hide resolved
@TedBrookings TedBrookings force-pushed the feature/specify_sort_order_in_sambuilder branch from 45ad823 to bd54520 Compare November 8, 2023 20:47
@TedBrookings
Copy link
Contributor

@nh13 In addition to the change you requested, I made three other notable changes

  1. I updated the poetry environment to succeed in more recent python environments.
  2. I removed the None option for sort_order. Unsorted and Unknown are both options and it feels messy to have None kicking around in there too.
  3. I allowed writing with Unknown sort order, since it's in the spec. It's the same as Unsorted in its effect. This feels marginal to me, and I think you might feel strongly about removing it, so I wanted to draw attention.

@@ -899,3 +900,14 @@ def __next__(self) -> Template:
name = self._iter.peek().query_name
recs = self._iter.takewhile(lambda r: r.query_name == name)
return Template.build(recs, validate=False)


class SamOrder(enum.Enum):
Copy link
Member

Choose a reason for hiding this comment

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

Having looked at this many months later, I wonder if this is too simplistic (see our Scala equivalent: https:/fulcrumgenomics/fgbio/blob/ff1ca67772c34a8defeb835d692a53ad6e536f74/src/main/scala/com/fulcrumgenomics/bam/api/SamOrder.scala#L172). It doesn't allow custom sort orders, including setting group order, sub-sort, etc. I am fine with this for now, but changing this later will require breaking changes, so lets require @tfenne to weigh in too.

Copy link
Member

Choose a reason for hiding this comment

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

I would stop where we are - there's nothing here that would prevent us from doing something more sophisticated later.

* Add SamOrder enum consistent with SAM spec
* Add _sort_order as private member of SamBuilder to avoid changes
  inconsistent with header
* Update poetry environment for newer python

Co-authored-by: Nathan Roach <[email protected]>
Co-authored-by: Ted Brookings <[email protected]>
@TedBrookings TedBrookings force-pushed the feature/specify_sort_order_in_sambuilder branch from bd54520 to ae5b948 Compare November 14, 2023 21:12
@TedBrookings TedBrookings merged commit ca4a572 into main Nov 14, 2023
6 checks passed
@TedBrookings TedBrookings deleted the feature/specify_sort_order_in_sambuilder branch November 14, 2023 21:35
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.

5 participants