[ai] Fixes by DavidLapous · Pull Request #1 · hschreiber/multipers

DavidLapous · 2026-10-09T16:44:26Z

Summary

Follow-up fixes for PR68, targeting its source branch hschreiber/multipers:simplex_tree_interface.

Correct SimplexTreeMulti binding semantics: GIL handling, argument order, real arithmetic, parameter widths, identity, and empty inputs.
Normalize ragged and absent Slicer lifetimes while preserving input ownership and CPU checks.
Fix empty Delaunay metadata/autograd, timer locking, and native conversion-symbol linking.
Add regression coverage and document the input contracts.

The 11 focused commits contain the detailed rationale and implementation changes.

Validation

Validated against PR68 head 838a13b:

Native rebuild, public-API smoke, 142 bounded CPU/MPS cases, strict conversion linking, and timer-concurrency probes passed.
Full suite: 297 passed, 2 failed. The failures concern incoming Graph-policy changes: Skyscraper's nonsquare-presentation recomputation and Slicer's AlphaComplex backend conversion. Neither test is skipped or altered to hide these failures.

Conversion templates instantiate in consumer extensions, including unoptimized builds where calls cannot disappear through inlining. Expose the conversion implementation symbols from the shared core so strict -O0/-fno-inline consumers can link without hidden-symbol errors.

Python simplex iterables can execute Python while vertices are read. Do not release the GIL around that traversal. Add coverage for list-based lookup and boundary retrieval.

The Python binding passes axis before the vertex filtration value. Match that order in the native signature rather than swapping the two values implicitly through scalar conversions. Cover fractional vertex values and preservation of the untouched axis.

Set the destination tree schema before copying a Slicer. Respect an explicit width override and otherwise inherit the source width, even when no simplices are available to infer it. Cover narrower, wider, and empty conversions.

Integer filtration storage must not truncate real grid coordinates or line parameters before native projection and coarsening. Bind these inputs as double and keep projected values in double. Add integer-tree regressions with fractional grids and non-unit directions.

Native modifiers return the existing tree, not an independently owned wrapper. Use reference_internal for mutation and conversion results so Python retains the original object and its lifetime. Cover normalization identity and mutated filtration values.

Keep a validated grid object even when its axes are empty; clearing the grid remains explicit. Return an empty tuple for an empty degree query. Treat an empty batch filtration as omitted grades after checking CPU storage, preserving the native minus-infinity default instead of parsing an empty matrix as a malformed grade. Cover all three cases.

Infer multi-critical width from a populated lifetime, not an empty first row. Keep the declared width for shaped empty inputs and reject widthless absent lifetimes explicitly. Normalize list and array forms consistently, retaining CPU checks at native ndarray access. Add empty-first, absent, and ragged regressions.

An empty triangulation still has one radius axis plus every function axis. Preserve that width in both native output forms rather than returning default-constructed metadata. Remove the stale Python width setter. Cover scalar and multi-function empty inputs, including tensor grids and gradients.

The timer mutex previously protected an empty block before computation. Hold it around the actual CoreDelaunay operation while the GIL is released, so enabled global timers cannot race between Python threads. Keep empty-input returns outside the native critical section.

Document real projection arithmetic, width overrides, empty metadata, absent versus omitted lifetimes, ragged corners, and input ownership. Retain the existing empty-Slicer limitation explicitly rather than promising inferred metadata for a genuinely empty complex.

hschreiber · 2026-10-12T16:16:29Z

-if(NOT MSVC)
-  # -fvisibility=hidden matches nanobind's own NB_NAMESPACE hidden-visibility
-  # attribute (GCC/Clang only) and silences the resulting -Wattributes.
-  # MSVC has no equivalent flag or warning here
-  target_compile_options(multipers_core_simplextree_obj PRIVATE -fvisibility=hidden -fvisibility-inlines-hidden)
-endif()


If I remember well, this was added to prevent some "declared with greater visibility than its base" and similar warnings. Can you confirm that those are gone and if not find a better solution for them?

hschreiber · 2026-10-12T16:39:01Z

 template <class Filtration_value>
 inline Filtration_value _cast_to_filtration_value(nanobind::object values, int defaultNumParam) {
  using value_type = typename Filtration_value::value_type;
+  if (nanobind::ndarray<> array; nanobind::try_cast(values, array, false)) _require_cpu_array(array);


This test is already done in the ndarray versions of _cast_to_filtration_value. The only possibility for a ndarray to not reach this test is if the ndim > 2 or ndim is 1 and the array is not continuous or U is some exotic type (i.e., only invalid input cases). So, it would be more interesting to add instead something like:

if (nanobind::ndarray<> val; nanobind::try_cast<nanobind::ndarray<>>(values, val, false)) throw std::invalid_argument("Wrong format for filtration value array.");

at line 554. It feels more coherent.

hschreiber · 2026-10-12T16:41:07Z

+      [&]() -> Filtration_value {
+        if (_sequence_size(values) != 0) return cast_as_vector();
+        return Filtration_value(defaultNumParam);
+      },


If we arrive here the sequence size is supposed to be 0. If it is not the case, then _dispatch_dtype has to be corrected to ensure it. Not here.

hschreiber · 2026-10-12T16:45:22Z

+    std::vector<Filtration_value> out(view.shape(0), Filtration_value::inf(view.shape(2)));
+    if (view.shape(1) == 0) return out;


@DavidLapous Can you confirm that we want empty arrays to yield infinity filtration values instead of ones with 0 generators? Or an error message?

hschreiber · 2026-10-12T16:46:42Z


 template <class Filtration_value>
 inline std::vector<Filtration_value> _cast_to_filtration_value_array(nanobind::object values, int defaultNumParam) {
+  if (nanobind::ndarray<> array; nanobind::try_cast(values, array, false)) _require_cpu_array(array);


Same comment than for than for line R554.

hschreiber · 2026-10-12T16:53:44Z

-      cast_first_as_tensor_then_as_vector,
-      []() -> std::vector<Filtration_value> { return {}; },
-      cast_as_vector);
+  return detail::_dispatch_dtype(values, cast_first_as_tensor_then_as_vector, cast_as_vector, cast_as_vector);


See comment for R658 to R686.

hschreiber · 2026-10-12T16:57:35Z

      nanobind::gil_scoped_release release;
      Base::clear();
      st = build_simplex_tree_from_complex<Options>(other.get_filtered_complex(), maxDim, numParam);
+      st.set_num_parameters(numParam >= 0 ? numParam : other.get_number_of_parameters());


I corrected this in build_simplex_tree_from_complex, so it can be removed from here.

hschreiber · 2026-10-12T16:59:36Z

-    // throws if it does not pass the check
-    // returns false if valid but empty
-    if (detail::_verify_grid_validity(grid)) {
-      filtrationGrid_ = grid;
-      return;
-    }
-
-    filtrationGrid_ = nanobind::none();
+    detail::_verify_grid_validity(grid);
+    filtrationGrid_ = grid;


That is not coherent with the behaviour in the Slicer. @DavidLapous can you confirm which version you want?

hschreiber · 2026-10-12T17:05:12Z

+      nanobind::ndarray<> array;
+      if (nanobind::try_cast(filtrationValues, array, false) && array.size() == 0) {
+        detail::_require_cpu_array(array);
+      } else {
+        fils = detail::_cast_to_filtration_value_array<Filtration_value>(filtrationValues, Base::num_parameters());
+      }


This test is already done in detail::_cast_to_filtration_value_array, what is the point of the branching? Avoid the machinery of detail::_cast_to_filtration_value_array? Is the runtime gain really worth it?
Also, if the array is empty, what is the point of the test? filtrationValues is never used again afterwards. Then it does not matter if it is cpu or not.

hschreiber · 2026-10-12T17:08:18Z

+           return self;
+         },
+         nb::rv_policy::reference_internal)
+      .def("_from_gudhi_state", &Interface::from_std, nb::rv_policy::reference_internal);


Is there really a reason to add nb::rv_policy::reference_internal here and everywhere below? If I remember well, when returning a reference from the binded class it-self it is not necessary. Or did I missed something?

DavidLapous added 11 commits October 9, 2026 18:22

fix(simplextree): keep GIL during iterable lookup

22fa248

Python simplex iterables can execute Python while vertices are read. Do not release the GIL around that traversal. Add coverage for list-based lookup and boundary retrieval.

fix(simplextree): preserve Slicer parameter widths

b814581

Set the destination tree schema before copying a Slicer. Respect an explicit width override and otherwise inherit the source width, even when no simplices are available to infer it. Cover narrower, wider, and empty conversions.

hschreiber requested changes Oct 12, 2026

View reviewed changes

Navigation Menu

Repository navigation

[ai] Fixes - #1

[ai] Fixes#1
DavidLapous wants to merge 11 commits into
hschreiber:simplex_tree_interfacefrom
DavidLapous:fix/pr68-native

DavidLapous commented Oct 9, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

hschreiber Oct 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		std::vector<Filtration_value> out(view.shape(0), Filtration_value::inf(view.shape(2)));
		if (view.shape(1) == 0) return out;

Conversation

DavidLapous commented Oct 9, 2026

Summary

Validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants