Repository navigation
Add Seeded Samplers - #537
Conversation
Marvin-Beckmann
left a comment
Marvin-Beckmann
left a comment
There was a problem hiding this comment.
Very elegant solution! I have a few design decisions one could consider, but the solution itself seems sound.
| #[cfg(test)] | ||
| mod test_sample_binomial_seeded { |
There was a problem hiding this comment.
A general suggestion, or rather a design decision:
Do we want to have the tests separated in modules? Alternatively, maybe it is easier to just loop over the different sampling methods, and check something like boundary checks. This ofc does not work for static tests for seeded samplers, but it could reduce maintainable code lines.
| /// Checks whether the same seed results in the same sample. | ||
| #[test] | ||
| fn same_seed_same_sample() { | ||
| use crate::integer::MatZ; | ||
| let sample_0 = MatZ::sample_binomial_seeded(2, 2, 5, 0.5, [42; 32]).unwrap(); | ||
| let sample_1 = MatZ::sample_binomial_seeded(2, 2, 5, 0.5, [42; 32]).unwrap(); | ||
|
|
||
| assert_eq!(sample_0, sample_1); | ||
| } |
There was a problem hiding this comment.
should these types of tests also be constant tests? I.e., not just test if the same seed gives the same value, but also that seeded sampling gives the same constant value?
| //! This module implements macros which are used to provide an unseeded and a | ||
| //! seeded variant of a function from one implementation and one doc-comment. | ||
|
|
||
| /// Takes a function `name` whose last parameter is an optional seed `seed: Option<[u8; 32]>` |
There was a problem hiding this comment.
The design decision as to why 32 Byte seeds are chosen is not completely clear: I would guess to have 256 bits, and it is fixed by the rng? One could otherwise consider just allowing to insert any type of Byte-Array too. I would just document the decision in the macro somewhere
| sigma: impl Into<f64>, | ||
| seed: Option<[u8; 32]>, | ||
| ) -> Result<MatQ, MathError> { | ||
| let mut rng = SamplerRng::new(seed); |
There was a problem hiding this comment.
for the developer it might ne be fully clear that this is how to use the sampler black-box in the construction, at least I did not find a comment on this in the macro file
2 participants
Description
This PR implements...
Testing
Checklist: