EddyRivasLab / hmmer

HMMER: biological sequence analysis using profile HMMs
http://hmmer.org
Other
317 stars 70 forks source link

Fix alimask out-of-range errors (iss #237) #258

Closed traviswheeler closed 3 years ago

traviswheeler commented 3 years ago

This commit addresses issue #237:

"Alimask crashes if asked to mask an alignment in a region that's outside the range of the alignment."

I've added range checks on all four of --alirange, --modelrange, --ali2model, and --model2ali. For all four:

For (--alirange, --ali2model): <to> can not exceed the alignment length.

For (--modelrange, --model2ali):

The --ali2model required a bit of extra attention: The purpose of this flag is to print, for each alignment range <from>..<to>, a mapping of the range with two pairs i..j => m..n, such that

There were two bugs in this code. The first allowed j to exceed <to> if the <to> column did not map to a model position. This is fixed. The second allowed a range-pair i..j => m..n to be produced even if no alignment columns in the range <from>..<to> map to a model position. I've revised the mask output to indicate this lack of mapped positions with an output like: <from>..<to> -> -..- (no map)

cryptogenomicon commented 3 years ago

Sounds good. @npcarter, could you have a look at this?

npcarter commented 3 years ago

Testing results: for all of --alirange, --modelrange, --ali2model, --model2ali, bounds checking works correctly.

--ali2model and --model2ali just output the conversions of the specified range to/from model space and did not generate an output msa file. Is this the correct behavior?

Valgrind reports memory leaks when run with any of the --alirange --modelrange --model2ali, --ali2model flags.

traviswheeler commented 3 years ago

The --ali2model and --model2ali behavior is expected. I'll look into the valgrind errors.

traviswheeler commented 3 years ago

@npcarter: This should be good for another check. I've cleaned out the memory errors (allocated vars not free'd at exit) and made a few small tweaks to the help text.

npcarter commented 3 years ago

Every test of this version fails with the following error:

[npcarter@eddyfs01 hmmer]$ src/alimask --alirange 0-20 ../sequence_dbs/test.msa ~/foo.out no such option --apendmask

traviswheeler commented 3 years ago

Sorry - the final changes I made (to account for re-spelling the appendmask flag) were sitting in a commit that I didn’t notice hadn't gone through. I’ve pushed them now.

npcarter commented 3 years ago

This version looks good to go. I've merged it into develop.