Make the surface samplers seedable (closes #54) - #55
Merged
Conversation
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.
Caught by CI. I ran black --check locally but not the repo's own `make lint` target, which runs isort first.
…es 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #54.
Mesh.rand_pts_around_surface()had two independent random draws that a caller could notreach, so identical inputs produced a different point cloud on every call. Both are now
seeded through a single optional
seedargument.Seeding only one of the two would have left the function random, which is the main reason
this was easy to miss — so every test here checks the whole call rather than one draw.
The change
rand_sample_pts_mesh(..., seed=None)passesrandom_seedto bothpcu.sample_mesh_randomandpcu.sample_mesh_poisson_disk.Mesh.rand_surface_pts(..., seed=None)forwards it.Mesh.rand_pts_around_surface(..., seed=None)usesnp.random.default_rng(seed)for theoffsets instead of
np.random.default_rng(), and forwards the seed to the surface draw.pcu_random_seed()pcu documents
random_seed=0as "use the current time", so a user's seed cannot behanded over directly:
seed=0would silently mean "unseeded", and 0 is the first seed mostpeople try.
pcu_random_seed()mapsNoneto pcu's own0sentinel (preserving theexisting default) and any integer seed to a deterministic non-zero 32-bit value.
Backwards compatibility
seed=Noneis the default and behaves exactly as before — unseeded, and unaffected bynp.random.seed(), which never reached either draw. There is a test asserting that,because it is the property a patch release must not break.
Verified, not assumed
seed=0twiceseed=Nonetwicedistribution="laplace"rand_sample_pts_mesh, bothrandomandbluenoise14 new tests in
testing/mesh/meshTools/rand_sample_pts_mesh_seed_test.py.One thing this does NOT fix
rand_pts_around_surface(surface_method="bluenoise")— the defaultsurface_method—is broken independently of seeding and remains broken:
pcu.sample_mesh_poisson_diskreturns approximatelynum_samplespoints (it took 1450for a requested 1000), and
base_pts + rand_ptsthen broadcasts mismatched shapes. Thisreproduces on
mainat 0.1.20, so it predates this branch. It is left alone deliberately:it is a different defect with a design question attached (truncate? oversample and trim?
raise?), and bundling it into a seeding patch release would hide it.
Consequence for this PR's coverage:
bluenoiseseeding is verified at therand_sample_pts_meshlevel, which is where the seed lands, but not end-to-end throughrand_pts_around_surface, because that path raises before it can be checked. The tests sayso at the point where it matters.
Version bumped 0.1.20 → 0.1.21.