Add a RandomState parameter to the clutter model #587
Conversation
Codecov Report
@@ Coverage Diff @@
## main #587 +/- ##
=======================================
Coverage 94.34% 94.34%
=======================================
Files 154 154
Lines 7344 7345 +1
Branches 1397 1397
=======================================
+ Hits 6929 6930 +1
Misses 313 313
Partials 102 102
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
stonesoup/models/clutter/clutter.py
Outdated
| @@ -42,6 +42,9 @@ class ClutterModel(Model, ABC): | |||
| "The default defines the space for a uniform distribution in 2D. The call " | |||
| "`np.array([self.distribution(*arg) for arg in self.dist_params])` " | |||
| "must return a numpy array of length equal to the number of dimensions.") | |||
| random_state: Optional[np.random.RandomState] = Property( | |||
| default=np.random.RandomState(1), | |||
There was a problem hiding this comment.
The issue with having this as a default, is run to run you'll get the same results, and I think it'd be better that by default the global random state is used. Also, this default will be shared by all instances, which may have unexpected issues.
Something like in Gaussian Model may be better:
Stone-Soup/stonesoup/models/base.py
Lines 211 to 215 in 2de2fc0
There was a problem hiding this comment.
That's a very good point, I hadn't thought of that. I've made this change like you suggested. However, I did keep the seed as either an integer seed or a numpy random state (or default None). I thought this way the user can use the same random state for the poisson random variable and the distribution attribute. For example:
rs = numpy.random.RandomState(seed=1)
clutter_model = ClutterModel(
distribution=rs.uniform,
seed=rs,
...
)
Does that seem ok?
There was a problem hiding this comment.
Sorry, I don't quite understand why this change made the tests fail. It seems to be failing Test 39, installing dependencies.
There was a problem hiding this comment.
Sorry, I don't quite understand why this change made the tests fail. It seems to be failing Test 39, installing dependencies.
CircleCI seems to be having issues lately with the dependency cache getting corrupted. Seems I can't reset the cache on PRs from forks. Will have to look into a solution, but just ignore failures for now.
There was a problem hiding this comment.
ok, thank you!
This reverts commit cbe65f4.
Fix a small bug in the random seed so that it uses the global random state as default.
Fix small bug so that the clutter model uses global random state as default.
Add an optional numpy.random.RandomState parameter to the clutter model. This helps with reproducibility of code and closes #575.