Skip to content

Conversation

@a1danbryant
Copy link

This PR contains the addition of tests/test-persistence.R, a test file to support the R/persistence-class.R file.
Tests include:

  • compatibility with persistence diagrams from TDA and ripserr (including version-specific code paths)
  • correct handling of multiple input types (matrix, list of matrices, etc.)
  • conversion between persistence objects and data.frame
  • behavior of the get_pairs()

A couple questions I have include:

  • Is there any additional edge cases we should include?
  • Are there any preferred style tweaks or conventions I should follow?

Looking forward to your feedback!

@corybrunson
Copy link
Member

@a1danbryant i believe the new test file should be located alongside the existing test files in inst/tinytest/. Once that's done, are you able to run the tests on your machine using the {tinytest} package?

It might eventually become part of test-persistence-class.R, though since both contain many tests they might be kept separate instead, since they cover somewhat different realms of behavior.

@astamm
Copy link
Collaborator

astamm commented May 13, 2025

Thanks for this PR.

The CI however fails. Can you please take a look?

@corybrunson
Copy link
Member

And, to be sure, these are mostly derived from the previous version of the "persistence" class from {plt}, correct? So it would not be surprising to find some incompatibilities with this iteration of the class.

@a1danbryant
Copy link
Author

a1danbryant commented May 14, 2025

And, to be sure, these are mostly derived from the previous version of the "persistence" class from {plt}, correct? So it would not be surprising to find some incompatibilities with this iteration of the class.

Yes, this has been adapted from the "persistence" from {plt}.

The CI however fails. Can you please take a look?

Looking into this now.

@a1danbryant
Copy link
Author

a1danbryant commented May 14, 2025

After making moving the test-persistence.R to the correct location I found the error you were receiving (as_persistence() not being found). I ran devtools::load_all and that fixed this error, however it modified the files: R/cpp11.R and src/cpp11.cpp. Does this seem correct?

Also, it seems running the tests through here causes the handling for the package version of {ripserr} to err. I'm looking into this error now.

@a1danbryant
Copy link
Author

Okay! Everything is working smoothly on my end now, sorry for the hold up! Let me know if any issues persist.

@astamm
Copy link
Collaborator

astamm commented May 21, 2025

Thanks!

It still does not pass the CI. Now because of:

Error in alphaComplexDiag(df3d, maxdimension = 1) : 
  could not find function "alphaComplexDiag"

@a1danbryant
Copy link
Author

Sorry about that-- I must have ran the test with the {TDA} package loaded.
With the above additions, here is the response from running:

tinytest::test_all("C:/Users/bryan/phutil/")

Console output:

> tinytest::test_all("C:/Users/bryan/phutil/")
test-distances.R..............   24 tests OK 1.1s
test-persistence-class.R......   44 tests OK 0.6s
test-persistence.R............   35 tests OK 44ms
test-utils.R..................    4 tests OK 
! Birth values are expected to be smaller than death values.
test-utils.R..................    5 tests OK 
! Birth values are expected to be smaller than death values.
test-utils.R..................    8 tests OK 0.2s
All ok, 111 results (2.0s)
<\details>``` 

@a1danbryant
Copy link
Author

Hey @astamm just shooting you a follow up about this.

@corybrunson
Copy link
Member

@astamm i don't think we can add {ripserr} to Suggests and later add {phutil} as a dependency of {ripserr}, as here. Could we use its output without the package itself, e.g. via dput()?

@astamm
Copy link
Collaborator

astamm commented Oct 1, 2025

We can skip tests relying on ripserr if not installed, which would avoid the suggested dependency.

@corybrunson
Copy link
Member

Is that right? I was under the impression it would still need to be listed somewhere in 'DESCRIPTION'. But if it works, great!

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.

3 participants