Skip to content

Conversation

@ABFritschi
Copy link
Collaborator

@ABFritschi ABFritschi commented Dec 1, 2025

Added the Mapper algorithm to ceREEBerus

Description

This change primarily consist of adding a new function, computeMapper, to ceREEBerus. This function computes the mapper algorithm used in TDA. It additionally implemented a new function, cover, to aid in the usage of the Mapper algorithm, and it moved the draw function to a new sub-module.

Motivation and Context

We have an existing MapperGraph class that is built on top of the ReebGraph class but without any way to compute Mapper Graphs using the Mapper algorithm. This change implements the Mapper algorithm into the package.

How has this been tested?

I have added three new unit tests for the code. One checking the the new cover function and two checking the computeMapper function. The computeMapper has a test for a simple trivial clustering graph for manually input points and covers, and a complicated graph test built on the sklearn.make_circles function on a set seed. Testing has been done for 2D and 3D point clouds.

The other change, moving the draw method to its own sub-module, has been checked for bugs as well as being moved in the documentation and currently no issues are found.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have incremented the version number in the pyproject.toml file if a new version needs to be pushed to pypi. Note that if the number isn't incremented, the package will not be pushed to pypi, which is useful if this PR is only for updating documentation.
  • My code follows the code style of this project and I have run make format to clean up the code with black.
  • My change requires a change to the documentation. I have updated the documentation as necessary and compiled locally to ensure it is clean.
  • I have added tests to cover my changes, and all new and existing tests passed (run make tests).

@yemeen
Copy link
Member

yemeen commented Dec 2, 2025

the last test needs sklearn to be added to requirements to run or a new function should be defined. a new function would be preferable as no other parts of the base uses sklearn.

Run source .venv/bin/activate
# Running unittests
E................................
======================================================================
ERROR: tests.test_computemapper (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_computemapper
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.10.19/x64/lib/python3.10/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/opt/hostedtoolcache/Python/3.10.19/x64/lib/python3.10/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/home/runner/work/ceREEBerus/ceREEBerus/tests/test_computemapper.py", line 4, in <module>
    from sklearn.datasets import make_circles
ModuleNotFoundError: No module named 'sklearn'

@ABFritschi
Copy link
Collaborator Author

Sorry, I forgot that sklearn isn't a dependency. I have been using it frequently for my testing, since the mapper algorithm needs a clustering algorithm to work.

There are a few ways to fix this without adding sklearn as a dependency that I can think of. I could just remove the failing test. The test is just a secondary test to ensure it works with clustering algorithms that are not the trivial clustering algorithm. The trivial clustering algorithm test is passing and ensures that the computeMapper method works. Another option could be using one of the clustering algorithms from scipy for the test, since scipy is currently listed as a requirement. Another idea could be that I implement some of the more popular clustering algorithms used with Mapper (DBScan seems like a first choice) into ceREEBerus to remove that dependency, though that would be quite a bit of work.

Let me know what you think the best way to solve the problem is.

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