Hi Lennart, thanks again for your analysis of the code.
On Tue, Sep 26, 2017 at 02:21:16PM -0400, Lennart Sorensen wrote: > On Tue, Sep 26, 2017 at 02:09:56PM -0400, wrote: > > > > > > At some point I introduced an additional letter to the alphabet, the X > > > > (a masking letter; see input.h). I am not sure about the program logic > > > > anymore, whether X can actually be used for lookups in the energy > > > > tables (as the N could). In hybrid_core.c, where the energy functions > > > > are used, I check for X in a few places, so the idea was perhaps to not > > > > use X for lookups (in fact, I do not want to consider any masked > > > > sequence at all). Should that be true, ALPHASIZE being 6 would be > > > > wasteful, but it shouldn't harm otherwise. Of course, you could change > > > > the two for-loops (the k-loop in init_dr_dangle_dg_ar and the i-loop in > > > > its sibling) so that they use < instead of <=, but, while that might > > > > fix the array bound bug, it might not solve an underlying logic > > > > problem. In effect you might have a program that works technically, but > > > > produces the wrong result. Unfortunately I can't help with that. You > > > > could try fixing the loops and then do a few runs and compare the > > > > outputs, as a minimal test. > > > > > > So it seems to me that it is safer to leave the loop untouched ... > > > > Makes sense if the energy.h is fixed instead. > > Of course it seems the change was a number of places. ALPHASIZE was > changed from 5 to 6, which removed the need for the +1 in a number of > places, but of course makes the arrays a chunk larger. > > I think in fact just fixing the two places that initialize the array to > 0 to not have the <= is in fact the safer option since that will match > the allocation. Everywhere else that had to deal with the +1 seems to > have already been changed in 2.1.2 to match ALPHASIZE now being 6. > > Of course the larger arrays might have something to do with why the > compile takes a bit longer than before. But that could also just be a > gcc change. To summarise: We should go with the patch you suggested originally https://anonscm.debian.org/git/debian-med/rnahybrid.git/tree/debian/patches/fix_loop_index.patch to fix the bug and in addition the warnings will be fixed in https://anonscm.debian.org/git/debian-med/rnahybrid.git/tree/debian/patches/fix_warnings.patch I'll be able to run the test suite on amd64 and also one arm64 box I own. Kind regards Andreas. -- http://fam-tille.de
