Skip to content

updated CubicalRipser version - #51

Open
veronicaoi wants to merge 5 commits into
tdaverse:mainfrom
veronicaoi:update_cubical
Open

veronicaoi wants to merge 5 commits into
tdaverse:mainfrom
veronicaoi:update_cubical

Conversation

@veronicaoi

Copy link
Copy Markdown

Updated the version of Cubical Ripser to the latest active development repository. Modifications to the source code include:

  • Removed outdated source files (cubical_2dim.cpp, cubical_3dim.cpp, cubical_4dim.cpp)
  • Replaced them with the latest source code directly from the CubicalRipser repo. This includes adding the following files:
    • dense_cubical_grids.h/cpp, cube.h, config.h, npy.hpp, compute_pairs.h/cpp, joint_pairs.h/cpp, ph_2d.h/cpp, coboundary_enumerator.h/cpp, union_find.h, radix_sort.h, representatives.h/cpp, write_pairs.h
  • Updated C++ binding layer to ensure compatibility with latest codebase by adding cubicalripser_bind.cpp, which parses R inputs and exports a function cubical_compute to convert results into a NumericMatrix
  • Refactored cubical.R to send inputs through cubical_compute instead of the dimension-specific wrapper functions
  • The following files were automatically updated as a result of these changes: DESCRIPTION, NAMESPACE, RcppExports.R/cpp

All the package tests appear to be passing; should close issue #50

@corybrunson

Copy link
Copy Markdown
Member

Thank you @veronicaoi ! I run a check locally and get the following warnings:

❯ checking if this is a source package ... WARNING
  Subdirectory ‘src’ contains:
    npy.hpp
  These are unlikely file names for src files.

This raises a broader issue: Are there header or source files among those you added from CubicalRipser that aren't essential for the R bindings? Even if so, we would want to delete sparingly, since every change from the original library makes it more difficult to upgrade in future.

❯ checking compiled code ... WARNING
  File ‘ripserr/libs/ripserr.so’:
    Found ‘__ZNSt3__14cerrE’, possibly from ‘std::cerr’ (C++)
      Object: ‘dense_cubical_grids.o’
    Found ‘__ZNSt3__14coutE’, possibly from ‘std::cout’ (C++)
      Objects: ‘compute_pairs.o’, ‘joint_pairs.o’, ‘ph_2d.o’
    Found ‘_exit’, possibly from ‘exit’ (C)
      Object: ‘joint_pairs.o’
  
  Compiled code should not call entry points which might terminate R nor
  write to stdout/stderr instead of to the console, nor use Fortran I/O
  nor system RNGs nor [v]sprintf.
  
  See ‘Writing portable packages’ in the ‘Writing R Extensions’ manual.

These files seem to be relics of the previous bindings; is that right? My guess is that the warning appears now because we last built this package using an older version of {devtools} (or its dependencies), but i thought i should ask.

I'll add comments about the functionality itself soon.

@corybrunson

Copy link
Copy Markdown
Member

I see a potential bug with 1-dimensional arrays; see below, first with main installed and second with update_cubical. Do you get the same results? (Sorry for forgetting set.seed().)

# `main` branch
library(ripserr)
dataset <- rnorm(1500)
dim(dataset) <- 1500
cubical(dataset)
#> PHom object containing persistence data for 488 features.
#> 
#> Contains:
#> * 488 0-dim features
#> 
#> Radius/diameter: min = -2.86; max = 3.6882.

Created on 2026-08-07 with reprex v2.1.1

# `update_cubical` branch
library(ripserr)
dataset <- rnorm(1500)
dim(dataset) <- 1500
cubical(dataset)
#> PHom object containing persistence data for 0 features.
#> 
#> Contains:

Created on 2026-08-07 with reprex v2.1.1

We had to use an R hack to get the previous version of CubicalRipser to treat them correctly; i think we fed CubicalRipser the dimension vector $(n,1)$, where $n$ was the number of cells in the 1-dimensional array.

@corybrunson

corybrunson commented Aug 9, 2026 •

Copy link
Copy Markdown
Member

The CubicalRipser README mentions that both V and T filtrations are supported. This raises two questions:

  1. Which filtration does the current PR use? (This will need to be documented.)
  2. Could the PR be revised to accommodate both filtrations? (This would be valuable because neither is available through any other R package.)

The syntax can be faithful to that of CubicalRipser, e.g. the R user would pass cubical(x, filtration = "V") and this would be passed internally to cubical_compute(as.vector(x), dim(x), filtration = "V").

Edit: Or maybe it's not an argument but a pre-compilation setting? (I'm looking here.) In that case, the challenge will be similar to that of enabling higher-order prime fields for Ripser; see #41 (and please comment there if i've misunderstood what C++ is doing).

@corybrunson

Copy link
Copy Markdown
Member

The 1D bug is resolved in 65a2cab.

@corybrunson

corybrunson commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

@veronicaoi please add yourself as an author—i suggest after Sean to keep the list sorted by relator and then chronologically. Or i will eventually unless you object.

Also, if you're up for it, write a brief entry in 'NEWS.md'. Or delegate it to me.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants