Skip to content

Interpolators cannot be downcast back to their derived type in Python #96

Description

@gagelarsen

Summary

When a BSHP<InterpBase> is returned to Python, pybind11 always yields the base InterpBase, never the derived InterpLinear / InterpIdw / InterpAnisotropic / InterpLinearExtrapIdw. Any Python code that dispatches on the concrete type therefore fails.

The symptom in xmsmesher:

RuntimeError: Unknown interp type: <class 'xms.interp._xmsinterp.interpolate.InterpBase'>
  _package/xms/mesher/meshing/poly_input.py:156

Root cause

XMS interpolators are abstract interfaces built by a New() factory:

class InterpLinear : public InterpBase
{
  static BSHP<InterpLinear> New();
  virtual ~InterpLinear();
  ...pure virtual...
};

So InterpLinear::New() returns a pointer whose dynamic type is InterpLinearImpl, defined in a .cpp and never registered with pybind11.

pybind's polymorphic caster does typeid(*src) to find the most-derived registered type. It finds InterpLinearImpl, which is not in the registry, and falls back to the static type of the expression — InterpBase. The py::class_<InterpLinear, InterpBase, BSHP<InterpLinear>> registration is correct and irrelevant: nothing ever produces a pointer whose dynamic type is InterpLinear.

This is platform-independent (reproduced on msvc 192, msvc 194, gcc 13 and apple-clang) and affects every interpolator type, not just one.

How long this has been latent

xmsmesher/_package/tests/unit_tests/multi_poly_mesher_io_pyt.py on master has four of these read-back assertions commented out:

# self.assertEqual(size_func.to_string(), pi.size_function.to_string())
# self.assertEqual(elev_func.to_string(), pi.elevation_function.to_string())
# self.assertEqual(str(size_func), str(pi.size_function))
# self.assertEqual(elev_func.to_string(), pi.elevation_function.to_string())

Someone hit this before and worked around it rather than fixing it, so PolyInput.size_function / .elevation_function have never actually worked for any type. It surfaced now because a new raster test asserted the read-back rather than commenting it out (Aquaveo/xmsmesher#86).

Proposed fix: a virtual type tag on InterpBase

/// \brief Name identifying the concrete interpolator type.
virtual std::string InterpTypeName() const = 0;   // "linear", "idw", "raster", ...

Python then dispatches on the tag instead of isinstance:

return _INTERP_WRAPPERS[size_func.interp_type_name](instance=size_func)

Why this one:

  • Immune to the pimpl pattern. It does not care that the dynamic type is an Impl.
  • Immune to cross-module registration. Works whether the derived class is bound in _xmsinterp or, like InterpRasterSizeFunction, in another module.
  • No pybind RTTI cooperation required, so it cannot regress on a compiler or visibility-flag change.

Alternatives considered

  • polymorphic_type_hook<InterpBase> specialization — pybind's idiomatic answer, but it requires naming the *Impl types, which live in .cpp files and are deliberately hidden. Fights the codebase's design.
  • Registering the Impl classes with pybind — leaks implementation types into the public Python API.
  • Caching the Python wrapper in PolyInput's setter — pure-Python and cheap, but only works when the object originated in Python; the C++ path stays broken.

Acceptance test

Re-enable all six assertions in multi_poly_mesher_io_pyt.py — the four commented out on master plus the two disabled in Aquaveo/xmsmesher#86 — and confirm they pass.

Blast radius

InterpBase is public API, so a new pure virtual is a breaking change for any external subclass. If that matters, give it a default implementation returning "unknown" and override it in each shipped interpolator.

Also note InterpRasterSizeFunction (xmsmesher) would need to implement the tag, so the two changes want coordinating.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions