rrrlw / TDAstats

R pipeline for computing persistent homology in topological data analysis. See https://doi.org/10.21105/joss.00860 for more details.
https://rrrlw.github.io/TDAstats
GNU General Public License v3.0
37 stars 8 forks source link

Support for prime coefficients #15

Closed peekxc closed 4 years ago

peekxc commented 4 years ago

See corresponding diff for summary of changes.

The new ripser_short.cpp swaps out the entry_t type to support coefficients in a prime field. (link to ripser code segment)

A parameter p is added to calculate_homology, which defaults to 2.

Additional tests were added to test functionality for the first five primes > 2 in test_ripser_accurate.R.

codecov-io commented 4 years ago

Codecov Report

Merging #15 into master will increase coverage by 0.19%. The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   93.37%   93.57%   +0.19%     
==========================================
  Files           6        6              
  Lines         589      576      -13     
==========================================
- Hits          550      539      -11     
+ Misses         39       37       -2
Impacted Files Coverage Δ
R/calculate.R 94.44% <66.66%> (-2.62%) :arrow_down:
src/ripser_short.cpp 93.54% <88.88%> (+1.09%) :arrow_up:
R/inference.R 93.67% <0%> (-0.88%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0700bff...8062aa4. Read the comment docs.

peekxc commented 4 years ago

Update on the PR:

The entry_t struct required using an (unsafe) gcc-specific packing macro intended to pack the coefficient at the end the entries index field so that entry_t structures are of the same size as index_t's.

The newest version of ripser has a compile-time assertion check that the packing indeed happens.

The problem I ran into is that 32-bit windows builds using the min-gw32 compiler by default doesn't support either the of the __attribute__((packed))(ref) or the more portable #pragma pack(1) (ref) preprocessor macros without some specific compiler flags (that would render TDAstats as non-portable on CRAN).

As far as I can tell, this is a bug in the mingw32 compiler (see the discussion here). The packing works successfully on e..g MSVC, gcc, and clang compilers. And the computation should still work for the 32-bit windows users compiling with mingw32, but their access time might be effected. Overall this seems like a minor issue.

peekxc commented 4 years ago

Temporarily taking this down to do some more testing and increase the coverage