From 13f6cfb972b546b97646960f6eee23589cfbfc80 Mon Sep 17 00:00:00 2001 From: anthony Date: Sun, 16 Aug 2026 02:57:39 +0000 Subject: [PATCH 1/3] Make the surface samplers seedable Closes #54. `Mesh.rand_pts_around_surface()` had two independent random draws a caller could not reach, so identical inputs gave a different point cloud on every call: - base surface points went to `pcu.sample_mesh_random` / `sample_mesh_poisson_disk` with no `random_seed`, and pcu documents `random_seed=0` as "use the current time", not "seed 0"; - the offsets used `np.random.default_rng()` with no argument, which seeds from OS entropy and ignores `np.random.seed()`. Both are now driven by one optional `seed`, threaded through `rand_sample_pts_mesh`, `Mesh.rand_surface_pts` and `Mesh.rand_pts_around_surface`. Seeding only one of the two would leave the function random, which is why this was easy to miss, so the tests check the whole call rather than a single draw. `pcu_random_seed()` maps None to pcu's own 0 sentinel and any integer seed to a deterministic non-zero 32-bit value -- otherwise `seed=0` would silently mean "unseeded", and 0 is the first seed most people try. `seed=None` is the default and behaves exactly as before; there is a test asserting that, because it is the property a patch release must not break. Not fixed here: `rand_pts_around_surface(surface_method="bluenoise")` -- the default surface_method -- raises a broadcast error because `sample_mesh_poisson_disk` returns approximately, not exactly, `num_samples` points. That reproduces on main at 0.1.20 and is a separate defect with a design question attached. Consequence: bluenoise seeding is verified at the `rand_sample_pts_mesh` level, where the seed lands, but not end-to-end. Full suite: 163 passed, 51 skipped, 0 failed. --- pymskt/__init__.py | 2 +- pymskt/mesh/meshTools.py | 30 ++++- pymskt/mesh/meshes.py | 31 ++++- .../rand_sample_pts_mesh_seed_test.py | 114 ++++++++++++++++++ 4 files changed, 167 insertions(+), 10 deletions(-) create mode 100644 testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py diff --git a/pymskt/__init__.py b/pymskt/__init__.py index c392fc4..84f74d6 100644 --- a/pymskt/__init__.py +++ b/pymskt/__init__.py @@ -12,4 +12,4 @@ RTOL = 1e-4 ATOL = 1e-5 -__version__ = "0.1.20" +__version__ = "0.1.21" diff --git a/pymskt/mesh/meshTools.py b/pymskt/mesh/meshTools.py index be93774..1d14fb4 100644 --- a/pymskt/mesh/meshTools.py +++ b/pymskt/mesh/meshTools.py @@ -1530,19 +1530,43 @@ def consistent_normals(mesh, points_dtype=np.float64, faces_dtype=np.int32): return new_mesh +def pcu_random_seed(seed): + """ + Translate a user seed into one `point_cloud_utils` will honour. + + pcu treats `random_seed=0` as "seed from the current time", not "seed 0" -- so a user + seed cannot be handed over directly. `seed=0` is the first seed most people try, and + passing it through would silently mean "unseeded". + + Returns 0 (pcu's own unseeded sentinel) when `seed is None`, so the default behaviour + is unchanged; otherwise a non-zero 32-bit integer derived deterministically from `seed`. + """ + if seed is None: + return 0 + return int(np.random.default_rng(seed).integers(1, 2**31 - 1)) + + def rand_sample_pts_mesh( - mesh, n_pts, method="bluenoise", points_dtype=np.float64, faces_dtype=np.int32 + mesh, n_pts, method="bluenoise", points_dtype=np.float64, faces_dtype=np.int32, seed=None ): """ Randomly sample points from a mesh + + Args: + seed (int, optional): Makes the sampling reproducible. Defaults to None, which + samples differently on every call -- the historical behaviour. """ # get faces and points faces, points = get_faces_vertices(mesh, points_dtype=points_dtype, faces_dtype=faces_dtype) + random_seed = pcu_random_seed(seed) + if method == "random": - fid, bc = pcu.sample_mesh_random(points, faces, n_pts) + fid, bc = pcu.sample_mesh_random(points, faces, n_pts, random_seed=random_seed) elif method == "bluenoise": - fid, bc = pcu.sample_mesh_poisson_disk(points, faces, num_samples=n_pts) + fid, bc = pcu.sample_mesh_poisson_disk( + points, faces, num_samples=n_pts, random_seed=random_seed + ) rand_pts = pcu.interpolate_barycentric_coords(faces, fid, bc, points) diff --git a/pymskt/mesh/meshes.py b/pymskt/mesh/meshes.py index 6faf901..8da294b 100644 --- a/pymskt/mesh/meshes.py +++ b/pymskt/mesh/meshes.py @@ -346,24 +346,43 @@ def get_largest(self): print(mesh_.n_points) self.deep_copy(mesh_) - def rand_surface_pts(self, n_pts=100_000, method="bluenoise"): + def rand_surface_pts(self, n_pts=100_000, method="bluenoise", seed=None): """ Sample points from the surface of the mesh. + + Args: + seed (int, optional): Makes the sampling reproducible. Defaults to None, which + samples differently on every call -- the historical behaviour. """ - return rand_sample_pts_mesh(self, n_pts=n_pts, method=method) + return rand_sample_pts_mesh(self, n_pts=n_pts, method=method, seed=seed) def rand_pts_around_surface( - self, n_pts=100_000, surface_method="bluenoise", distribution="normal", sigma=1.0 + self, + n_pts=100_000, + surface_method="bluenoise", + distribution="normal", + sigma=1.0, + seed=None, ): """ Sample points around the surface of the mesh. For SDF sampling & neural implicit representation models. + + Args: + seed (int, optional): Makes the sampling reproducible. Defaults to None, which + samples differently on every call -- the historical behaviour. + + Notes: + Both draws here are seeded: the base surface points (via `rand_surface_pts`) + and the offsets applied to them. Seeding only one leaves the result random. """ + rng = np.random.default_rng(seed) + if distribution == "normal": - rand_gen = np.random.default_rng().multivariate_normal + rand_gen = rng.multivariate_normal elif distribution == "laplace": - rand_gen = np.random.default_rng().laplace + rand_gen = rng.laplace - base_pts = self.rand_surface_pts(n_pts=n_pts, method=surface_method) + base_pts = self.rand_surface_pts(n_pts=n_pts, method=surface_method, seed=seed) mean = [0, 0, 0] if (distribution == "normal") and (sigma is not None): diff --git a/testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py b/testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py new file mode 100644 index 0000000..dd1aefc --- /dev/null +++ b/testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py @@ -0,0 +1,114 @@ +""" +Reproducibility of the random surface samplers. + +Regression tests for gattia/pymskt#54: ``Mesh.rand_pts_around_surface`` had two +independent random draws that a caller could not reach, so identical inputs produced +different point clouds on every call. + +* the base surface points went to ``pcu.sample_mesh_random`` / ``sample_mesh_poisson_disk`` + with no ``random_seed``, and pcu's ``random_seed=0`` default means "seed from the current + time", not "seed 0"; +* the offsets used ``np.random.default_rng()`` with no argument, which seeds itself from OS + entropy and ignores ``np.random.seed()``. + +Seeding only one of the two leaves the result random, so every test here checks the whole +call rather than a single draw. +""" + +import numpy as np +import pyvista as pv +import pytest + +from pymskt.mesh import Mesh +from pymskt.mesh.meshTools import pcu_random_seed, rand_sample_pts_mesh + + +@pytest.fixture +def sphere(tmp_path): + path = str(tmp_path / "sphere.vtk") + pv.Sphere(radius=1.0, theta_resolution=24, phi_resolution=24).triangulate().save(path) + return path + + +def around(path, seed, method="random", distribution="normal", n_pts=500): + return Mesh(path).rand_pts_around_surface( + n_pts=n_pts, surface_method=method, distribution=distribution, sigma=0.01, seed=seed + ) + + +class TestRandPtsAroundSurface: + def test_the_same_seed_gives_the_same_points(self, sphere): + assert np.array_equal(around(sphere, 42), around(sphere, 42)) + + def test_different_seeds_give_different_points(self, sphere): + assert not np.array_equal(around(sphere, 42), around(sphere, 7)) + + def test_seed_zero_is_reproducible(self, sphere): + """ + The one that would slip through. pcu reads ``random_seed=0`` as "use the clock", so + passing a user's seed straight through would make ``seed=0`` silently mean + "unseeded" -- and 0 is the first seed most people try. + """ + assert np.array_equal(around(sphere, 0), around(sphere, 0)) + + def test_the_default_is_still_unseeded(self, sphere): + """ + Backwards compatibility: ``seed=None`` must behave exactly as before, including + being immune to ``np.random.seed()``, which never reached either draw. + """ + np.random.seed(0) + first = around(sphere, None) + np.random.seed(0) + second = around(sphere, None) + assert not np.array_equal(first, second) + + def test_the_laplace_distribution_is_seeded_too(self, sphere): + assert np.array_equal( + around(sphere, 5, distribution="laplace"), + around(sphere, 5, distribution="laplace"), + ) + + +class TestRandSamplePtsMesh: + """ + The surface-point draw on its own. Both methods are covered here; ``bluenoise`` is not + covered end-to-end through ``rand_pts_around_surface`` because that path is broken for + an unrelated reason -- ``sample_mesh_poisson_disk`` returns approximately, not exactly, + ``num_samples`` points, so adding the offsets raises a broadcast error. That is a + separate defect and is not addressed here. + """ + + @pytest.mark.parametrize("method", ["random", "bluenoise"]) + def test_the_same_seed_gives_the_same_points(self, sphere, method): + mesh = Mesh(sphere) + first = rand_sample_pts_mesh(mesh, n_pts=400, method=method, seed=11) + second = rand_sample_pts_mesh(mesh, n_pts=400, method=method, seed=11) + assert np.array_equal(first, second) + + @pytest.mark.parametrize("method", ["random", "bluenoise"]) + def test_different_seeds_give_different_points(self, sphere, method): + mesh = Mesh(sphere) + first = rand_sample_pts_mesh(mesh, n_pts=400, method=method, seed=11) + second = rand_sample_pts_mesh(mesh, n_pts=400, method=method, seed=12) + assert first.shape != second.shape or not np.array_equal(first, second) + + +class TestPcuRandomSeed: + def test_none_maps_to_pcus_unseeded_sentinel(self): + assert pcu_random_seed(None) == 0 + + def test_zero_does_not_map_to_zero(self): + """Otherwise ``seed=0`` would mean "seed from the clock" inside pcu.""" + assert pcu_random_seed(0) != 0 + + def test_it_is_deterministic(self): + assert pcu_random_seed(3) == pcu_random_seed(3) + + def test_distinct_seeds_map_to_distinct_values(self): + assert len({pcu_random_seed(i) for i in range(25)}) == 25 + + def test_the_result_is_a_positive_32_bit_int(self): + for seed in (0, 1, 12345, 2**31): + value = pcu_random_seed(seed) + assert isinstance(value, int) + assert 0 < value < 2**31 From 81e8815110dcc2771140f9e005b8c544cc6fd013 Mon Sep 17 00:00:00 2001 From: anthony Date: Sun, 16 Aug 2026 02:59:26 +0000 Subject: [PATCH 2/3] Sort imports in the new test to satisfy isort Caught by CI. I ran black --check locally but not the repo's own `make lint` target, which runs isort first. --- testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py b/testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py index dd1aefc..248d876 100644 --- a/testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py +++ b/testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py @@ -16,8 +16,8 @@ """ import numpy as np -import pyvista as pv import pytest +import pyvista as pv from pymskt.mesh import Mesh from pymskt.mesh.meshTools import pcu_random_seed, rand_sample_pts_mesh From 5b6f3b5ca0d96bac21edcfcb3854e60a595a8500 Mon Sep 17 00:00:00 2001 From: anthony Date: Sun, 16 Aug 2026 03:26:09 +0000 Subject: [PATCH 3/3] Lead pcu_random_seed's docstring with why it derives rather than passes through The maintainer read the previous version as 'converts None to 0'. That is the trivial half; the derivation is what stops seed=0 meaning unseeded, keeps seeds 0 and 1 distinct, and keeps a large seed inside pcu's 32-bit binding. --- pymskt/mesh/meshTools.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/pymskt/mesh/meshTools.py b/pymskt/mesh/meshTools.py index 1d14fb4..4dc6877 100644 --- a/pymskt/mesh/meshTools.py +++ b/pymskt/mesh/meshTools.py @@ -1532,14 +1532,23 @@ def consistent_normals(mesh, points_dtype=np.float64, faces_dtype=np.int32): def pcu_random_seed(seed): """ - Translate a user seed into one `point_cloud_utils` will honour. + Derive a seed `point_cloud_utils` will honour, from one a user would write. - pcu treats `random_seed=0` as "seed from the current time", not "seed 0" -- so a user - seed cannot be handed over directly. `seed=0` is the first seed most people try, and - passing it through would silently mean "unseeded". + pcu's `random_seed` is not an ordinary seed argument, so a user seed cannot be passed + through. Three distinct problems, which is why this is a derivation and not a special + case: - Returns 0 (pcu's own unseeded sentinel) when `seed is None`, so the default behaviour - is unchanged; otherwise a non-zero 32-bit integer derived deterministically from `seed`. + - `random_seed=0` means "seed from the current time", not "seed 0". Passing a user's + 0 through would silently mean "unseeded" -- and 0 is the first seed most people try. + - Remapping only 0 to some constant would make that constant collide with a real seed: + `if seed == 0: seed = 1` gives seeds 0 and 1 the same run. + - The binding is a 32-bit C++ signature and raises TypeError outside that range, so a + timestamp or a hash used as a seed fails. + + Deriving a fresh non-zero 32-bit value from `seed` handles all three under one rule. + Negative seeds raise, inheriting numpy's contract rather than inventing another. + + `None` maps to pcu's own 0 sentinel, leaving the unseeded default untouched. """ if seed is None: return 0