Skip to content

Replace bam_construct_seq() with bam_set1()#1164

Closed
anderskaplan wants to merge 2 commits intosamtools:developfrom
anderskaplan:replace-bam_construct_seq
Closed

Replace bam_construct_seq() with bam_set1()#1164
anderskaplan wants to merge 2 commits intosamtools:developfrom
anderskaplan:replace-bam_construct_seq

Conversation

@anderskaplan
Copy link
Copy Markdown
Contributor

This is the continuation of #1159. It changes cram_to_bam() to call bam_set1() instead of bam_construct_seq(), fixes up cram_to_bam()'s return value (aux data length counted twice), and removes the now redundant cram/cram_samtools.c.

Note: there are other declarations in cram_samtools.h which maybe should be removed as well, such as the bam_seq_t type alias. Please advise!

@anderskaplan
Copy link
Copy Markdown
Contributor Author

The failed appveyor build does not seem related to the code changes, and unfortunately I don't see any way to retrigger the build.

@jkbonfield
Copy link
Copy Markdown
Contributor

The origin of cram_samtools.h is it's a wrapper file making io_lib's CRAM code work with htslib/samtools APIs and types. This cram code originated here: https://github.com/jkbonfield/io_lib/tree/master/io_lib

Initially we wanted to keep the code between the two libraries as consistent as possible, while development and bug fixes got worked on, but it's just diverged too much for that to really be a concern any more. Certainly once I went through replacing all use of my hash table with Heng's it was already looking radically different. Now it's just not worth worry about.

So the different data types and API names probably needs fixing at some point, but I'd argue it's a different issue. At least its own commit(s), but possibly a new PR too.

I'll take a look at the changes here, but I'm sure it's pretty straight forward.

Thanks for your work on this.

Removed redundant computation of the return value.
@jkbonfield
Copy link
Copy Markdown
Contributor

Thanks for this. I took the liberty of amending your commit to include removing cram_samtools.c from htslib.mk too.

Rebased, squashed and merged as 44787d9

@jkbonfield jkbonfield closed this Jan 6, 2021
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.

3 participants