fix(apps/nuclick): replace np.random.choice with self.R.choice in Add…#8836
fix(apps/nuclick): replace np.random.choice with self.R.choice in Add…#8836Zeesejo wants to merge 1 commit intoProject-MONAI:devfrom
Conversation
…PointGuidanceSignald In AddPointGuidanceSignald.exclusion_map, two calls to np.random.choice were using the global NumPy random state instead of the instance's self.R (RandomState). Since AddPointGuidanceSignald already inherits from Randomizable, replacing with self.R.choice ensures reproducibility when set_determinism() or manual seeding is used. Ref Project-MONAI#6888 Signed-off-by: Zeeshan Modi <92383127+Zeesejo@users.noreply.github.com> Signed-off-by: Zeeshan Modi <92383127+Zeesejo@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates NuClick’s AddPointGuidanceSignald transform to use its local MONAI Randomizable RNG (self.R) instead of NumPy’s global RNG for drop decisions, improving reproducibility when seeding is used via set_random_state() / Compose.set_random_state().
Changes:
- Replaced two
np.random.choice(...)calls withself.R.choice(...)insideAddPointGuidanceSignald.exclusion_map.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.R.choice([True, False], p=[drop_rate, 1 - drop_rate]): | ||
| return point_mask |
There was a problem hiding this comment.
This change is intended to improve reproducibility by using the transform's local RandomState, but there’s no regression test covering deterministic behavior. Consider adding a unit test (e.g., in tests/apps/nuclick/test_nuclick_transforms.py) that sets AddPointGuidanceSignald.set_random_state(seed=...) (and/or runs under Compose.set_random_state) and asserts the produced exclusion map / output image is identical across repeated calls with the same seed and differs with a different seed.
Fixes #6888 (partial) - follow-up to #8798
Description
In
AddPointGuidanceSignald.exclusion_map(monai/apps/nuclick/transforms.py), two calls usednp.random.choice(global NumPy random state) instead ofself.R.choice. SinceAddPointGuidanceSignaldalready inherits fromRandomizable, this is a straightforward replacement that ensures reproducibility whenset_determinism()or manual seeding is used.Before:
After:
PR #8798 already fixed
np.random.*calls in transforms and data utilities. This PR continues that work for the two remaining calls inapps/nuclick/transforms.pymentioned in #6888.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.