bamtools sort

[riedl]

In this implementation all sorting criterias have been excluded to the class api/BamSortCriteria. This has the advantage that any other sorting criterias can be easily added. Furthermore sorting can be switched to descending by adding -desc. This works for all sorting criterias. In this implementation sorting for the SAM/BAM tags QNAME, POS, AS (Alignment Score from BFAST Alignment) are possible

[pezmaster31]

Martin,
This looks good, definitely moving in the right direction. Huge thanks for contributing this.

However, as BamTools is primarily a library, I'd rather we not change the signature for the public BamMultiReader::SetSortOrder() API method if possible. I just pushed a source-breaking update last week and am wary to force another on clients so soon. Perhaps adding additional values to the SortOrder enum and then adding an optional boolean parameter (ascending?) to the SetSortOrder() method would capture the full range of current sort options for now. And then later on (maybe at BT 2.0), use your new BamSortCriteria object directly instead of these parameters.

So it could look like this (for now):
void BamMultiReader::SetSortOrder(const BamMultiReader::SortOrder& order, bool ascending = true);

This method would internally handle setting up the proper BamSortCriteria object to use. Just about everything else could remain.

It does look good, I just have to keep an eye on keeping the interface level as stable as possible. Feel free to give your response if you disagree or have other suggestions. Discussion is a good thing.

ps - Just got your email. Can you update this request when you have those sorting bugs ironed out? And perhaps the change I proposed to the API-level method? Again, huge thanks.

[riedl]

Hi Derek,

I can understand you, that you want to hold your BamMultiReader as stable as
possible :)

What do you think of having two signatures:
void BamMultiReader::SetSortOrder(const BamMultiReader::SortOrder& order,
bool ascending = true);
void BamMultiReader::SetSortOrder(const BamSortCriteria& sortCriteria);

The advantage would be, that the bamtools_sort class could then use all the
features given by the SortOrder class. Or do you also want the bamtools_sort
class as is ?

The error has been found in the Comparator functor used in the std::sort
function. The problem is that the functor expects compare(x,x) to be false.
This is no longer the case if I just invert the SortLessThan* class. So I had to
implement the SortGreaterThan* classes. Maybe I'll find some solution that is
more beauty.

Regards and thanks for offering this library

Martin

P.S.: The API adjustment will have been concluded after I sent this mail.

----- Ursprüngliche Mail ----
Von: pezmaster31
reply@reply.github.com
An: ma_riedl@yahoo.de
Gesendet: Dienstag, den 29. März 2011, 17:00:35 Uhr
Betreff: Re: [GitHub] bamtools sort [pezmaster31/bamtools GH-7]

Martin,
This looks good, definitely moving in the right direction. Huge thanks for
contributing this.

However, as BamTools is primarily a library, I'd rather we not change the
signature for the public BamMultiReader::SetSortOrder() API method if possible.
I just pushed a source-breaking update last week and am wary to force another on
clients so soon. Perhaps adding additional values to the SortOrder enum and then
adding an optional boolean parameter (ascending?) to the SetSortOrder() method
would capture the full range of current sort options for now. And then later on
(maybe at BT 2.0), use your new BamSortCriteria object directly instead of these
parameters.

So it could look like this (for now):
void BamMultiReader::SetSortOrder(const BamMultiReader::SortOrder& order,
bool ascending = true);

This method would internally handle setting up the proper BamSortCriteria object
to use. Just about everything else could remain.

It does look good, I just have to keep an eye on keeping the interface level as
stable as possible. Feel free to give your response if you disagree or have
other suggestions. Discussion is a good thing.

ps - Just got your email. Can you update this request when you have those
sorting bugs ironed out? And perhaps the change I proposed to the API-level
method? Again, huge thanks.

Reply to this email directly or view it on GitHub:
#7 (comment)

[pezmaster31]

Thanks for the update.

I have no problem adding your new overloaded method with BamSortCriteria to BMR; just didn't want to remove/modify the existing one in a way that would break source-compatibility.

The bamtools_sort tool can change as much as you want/need. There is no real API for it (so no code depends on it) and no user will "care" how the internals are implemented, as long as the utility works correctly.

So yeah, thanks again. I'll do a detailed review and merge soon.

[riedl]

Hi Derek

now everything seems to work correctly.Even I could simplify the sourcecode
again.

Have fun with the review. I hope you haven't started to far.

All the best
Martin

----- Ursprüngliche Mail ----
Von: pezmaster31
reply@reply.github.com
An: ma_riedl@yahoo.de
Gesendet: Mittwoch, den 30. März 2011, 17:00:15 Uhr
Betreff: Re: [GitHub] bamtools sort [pezmaster31/bamtools GH-7]

Thanks for the update.

I have no problem adding your new overloaded method with BamSortCriteria to BMR;
just didn't want to remove/modify the existing one in a way that would break
source-compatibility.

The bamtools_sort tool can change as much as you want/need. There is no real
API for it (so no code depends on it) and no user will "care" how the internals
are implemented, as long as the utility works correctly.

So yeah, thanks again. I'll do a detailed review and merge soon.

Reply to this email directly or view it on GitHub:
#7 (comment)