Fix computereeb reset pos performance

[ishikaghosh2201]

Description

computeReeb builds a Reeb graph one node/edge at a time but never overrode add_node/add_edge's reset_pos default of True, so every single insertion triggered a full set_pos_from_f() layout pass (a scipy.optimize.minimize L-BFGS-B call) over the entire graph built so far — O(N) full layout recomputations instead of one. This PR:

• Changes computeReeb to pass reset_pos=False on every internal add_node/add_edge call, and calls R.set_pos_from_f() exactly once after the graph is fully built.
• Fixes two related bugs in ReebGraph uncovered while verifying the above didn't change the graph's mathematical structure:
  • add_edge's tied-filtration-value collapse branch now forwards reset_pos to its internal recursive add_edge/remove_node calls instead of silently defaulting to True and doing an unwanted recompute regardless of what the caller asked for.
  • remove_node no longer bundles the pos_f dict cleanup inside if reset_pos:. Deleting a removed vertex's position entry is O(1) bookkeeping, not the expensive part, and gating it behind reset_pos left a dangling pos_f entry (pointing at a vertex no longer in the graph) whenever remove_node(..., reset_pos=False) was used — e.g. from remove_nodes_from or the add_edge collapse path above.

No public API changes; reset_pos keyword and its default (True) are unchanged everywhere.

Motivation and Context

Fixes #107. On a 287-node test graph (49-vertex triangle-mesh patch), computeReeb took 124s; profiling showed reeb_x_layout/scipy.optimize.minimize as the dominant cost, called 574 times for a graph with only 287 nodes. The fix brings this down to ~0.5s (~200x speedup) with an identical resulting graph.

How has this been tested?

• Verified structural equivalence directly, not just node/edge counts: ran the original and patched computeReeb on the same input and compared the full node set, full (directed) edge set, and f dict for exact equality (not just cardinality or sorted values) — all identical.
• Added regression tests to tests/test_lowerstar_class.py:
  • test_computeReeb_pos_f_consistency and test_computeReeb_torus_pos_f_consistency: after computeReeb, nodes/f.keys()/pos_f.keys() all match exactly, every pos_f[v][1] == f[v], and every edge points to the higher function value.
  • test_computeReeb_performance: asserts computeReeb on a 176-node torus grid completes in well under 5s (fixed version: ~0.4s; unpatched version: ~9s on the same input), guarding against the O(N) behavior being reintroduced.
• Added regression tests to tests/test_reeb_class.py:
  • test_remove_node_deferred_pos_cleanup: confirms remove_node(v, reset_pos=False) drops v's stale pos_f entry immediately rather than leaving it dangling.
  • test_add_edge_collapse_respects_reset_pos_false: builds a graph that exercises add_edge's tied-value collapse branch entirely with reset_pos=False, and confirms pos_f stays untouched (empty) until an explicit set_pos_from_f() call, i.e. the internal recursive calls correctly forward reset_pos instead of defaulting to True.
• Ran the full existing suite (make tests): all 68 tests (63 pre-existing + 5 new) pass.
• Audited every other call site of add_edge/remove_node in the source tree (not just tests) to confirm the change is behavior-preserving for the default reset_pos=True case (the vast majority of callers) and only changes behavior — strictly for the better — where reset_pos=False is already used. Confirmed the two subclasses with custom overrides, MapperGraph and MergeTree, both just forward reset_pos to super() and are already exercised by pre-existing tests that check the exact invariants this PR protects (test_mapper_class.py::test_add_edge's same-value collapse case, and test_merge_tree.py's check_mt helper asserting nodes == pos_f.keys()).

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).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

🟡 Changes recommended

Deferred node removal leaves a dangling self.pos entry; two minor test/comment fixes are also noted.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

tests/test_reeb_class.py:318

• This call does not exercise the tied-filtration collapse branch: a has value 0.0 while c has value 1.0, so add_edge('a', 'c') takes the ordinary edge path. Consequently this regression test would still pass if the collapse branch reverted to calling its internal methods with the default reset_pos=True; build an edge into/out of c and call add_edge('b', 'c', reset_pos=False) so the equal-valued collapse and its recursive calls are actually covered.

        R.add_edge('a', 'c', reset_pos=False)

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

[lizliz]

So much faster!