EddyRivasLab / easel

Sequence analysis library used by Eddy/Rivas lab code
Other
46 stars 26 forks source link

PPC fix, IEEE comparison check, isnanf check, travis test/badge #30

Closed horta closed 5 years ago

horta commented 5 years ago

The travis tests are now passing on the PPC 32-bits platform. Under that platform (at least when emulated via qemu), nan == nan fail, isnanf won't identify NAN when gcc optimization flag is used (gcc 4.9). I have decreased the number of trials for some esl_stats.c test as it was very slow to run that test on a slow, emulated machine. I also removed trailing whitespaces from some files.

cryptogenomicon commented 5 years ago

Can you say more about the test failures you saw, and how you're trying to work around them? If a system is failing tests for IEEE754-compliant math, I think we do want those tests to fail. Easel-based software (HMMER, Infernal) assumes IEEE754 compliance. Seems like allowing such tests to pass invites obscure bugs elsewhere in our code.

horta commented 5 years ago

Sure. The problem I saw was that (nanf == nanf) was true for when float nanf = NAN;. We can see that hapenning in an emulated ppc32 big endian platform: http://rest.s3for.me/hmmer/debian-wheezy-powerpc.qcow2

Here is the example:

root@debian-powerpc:~# cat test.c
#include <stdio.h>
#include <math.h>

int main()
{  float nanf = NAN;
  int status = nanf == nanf;
  printf("status: %d\n", status);
  return 0;
}
root@debian-powerpc:~# gcc test.c -o test && ./test
status: 1
root@debian-powerpc:~# gcc --version
gcc (Debian 4.9.1-19) 4.9.1
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
root@debian-powerpc:~# uname -a
Linux debian-powerpc 3.16.0-4-powerpc #1 Debian 3.16.7-ckt2-1 (2014-12-08) ppc GNU/Linux
root@debian-powerpc:~# cat /proc/cpuinfo
processor       : 0cpu             : 740/750
temperature     : 50-126 C (uncalibrated)clock           : 266.000000MHz
revision        : 3.1 (pvr 0008 0301)
bogomips        : 32.76
timebase        : 16383850
platform        : PowerMac
model           : Power Macintosh
machine         : Power Macintosh
motherboard     : AAPL,PowerMac G3 MacRISC
detected as     : 49 (PowerMac G3 (Silk))
pmac flags      : 00000000
pmac-generation : OldWorld
Memory          : 512 MB
root@debian-powerpc:~# 

I've consulted the PowerPC 7xx manual and they claim to be compatible. So there is two possibilities I think: (1) the emulation is what is causing problem; (2) that architecture is not 100% compliant.

cryptogenomicon commented 5 years ago

I'm inclined to treat that as a bug in the system somewhere (compiler, emulator...), and maybe you could figure out the right place to report it to. I don't think we want to tolerate reporting NaN == NaN as true; it violates IEEE754 and we're documented as assuming IEEE754 compatibility. I think the correct behavior for us is to fail in the test suite.

horta commented 5 years ago

That sounds fair. I will have a closer look into the system itself. I think it would be useful to have a working ppc emulation to have testing on ppc easier to perform. (i've never used an actualy ppc platform .)

cryptogenomicon commented 5 years ago

Rather than accepting the entire pull request, in f18a539 I've brought in changes to add basic continuous integration testing with travis-ci, based on your code.

I would rather keep that testing simple for the moment, and not bring in Docker-based tests on other platforms. At least for now, the level of complexity of your scripting for this is a bit higher than what I'm able to understand and maintain. We can revisit this in the future, as I get more comfortable!

Let me know if f18a539 is insufficient for what you were trying to add.

Thanks!

horta commented 5 years ago

Thanks Sean! I'm working on the complexity of the test and that IEEE problem. I might try another pull request tomorrow. (Please, don't feel urged to review my pull requests so quickly...)