-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Statistical assessment of spatial distributions #12835
base: main
Are you sure you want to change the base?
Conversation
Chi-squared, Nice! Looks pretty good at a glance, I will have time to review later in the week. |
Stolen from #12835. # Objective Sometimes you want to sample a whole bunch of points from a shape instead of just one. You can write your own loop to do this, but it's really more idiomatic to use a `rand` [`Distribution`](https://docs.rs/rand/latest/rand/distributions/trait.Distribution.html) with the `sample_iter` method. Distributions also support other useful things like mapping, and they are suitable as generic items for consumption by other APIs. ## Solution `ShapeSample` has been given two new automatic trait methods, `interior_dist` and `boundary_dist`. They both have similar signatures (recall that `Output` is the output type for `ShapeSample`): ```rust fn interior_dist(self) -> impl Distribution<Self::Output> where Self: Sized { //... } ``` These have default implementations which are powered by wrapper structs `InteriorOf` and `BoundaryOf` that actually implement `Distribution` — the implementations effectively just call `ShapeSample::sample_interior` and `ShapeSample::sample_boundary` on the contained type. The upshot is that this allows iteration as follows: ```rust // Get an iterator over boundary points of a rectangle: let rectangle = Rectangle::new(1.0, 2.0); let boundary_iter = rectangle.boundary_dist().sample_iter(rng); // Collect a bunch of boundary points at once: let boundary_pts: Vec<Vec2> = boundary_iter.take(1000).collect(); ``` Alternatively, you can use `InteriorOf`/`BoundaryOf` explicitly to similar effect: ```rust let boundary_pts: Vec<Vec2> = BoundaryOf(rectangle).sample_iter(rng).take(1000).collect(); ``` --- ## Changelog - Added `InteriorOf` and `BoundaryOf` distribution wrapper structs in `bevy_math::sampling::shape_sampling`. - Added `interior_dist` and `boundary_dist` automatic trait methods to `ShapeSample`. - Made `shape_sampling` module public with explanatory documentation. --- ## Discussion ### Design choices The main point of interest here is just the choice of `impl Distribution` instead of explicitly using `InteriorOf`/`BoundaryOf` return types for `interior_dist` and `boundary_dist`. The reason for this choice is that it allows future optimizations for repeated sampling — for example, instead of just wrapping the base type, `interior_dist`/`boundary_dist` could construct auxiliary data that is held over between sampling operations.
I'm thinking this code/module should probably be behind a feature flag. I'm not convinced bevy devs need this compiled into their apps by default. |
#[derive(Debug, Error)] | ||
#[error("One or more provided dimensions {dimensions:?} was outside of the range 0..{ambient_dimension}")] | ||
pub struct InvalidDimensionError { | ||
ambient_dimension: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fields should be pub.
/// of freedom exactly; as a result, index zero is just a placeholder value. | ||
/// | ||
/// Source: [NIST](https://www.itl.nist.gov/div898/handbook/eda/section3/eda3674.htm) | ||
pub const CHI_SQUARED_CRIT_VALUES_EMINUS3: [f64; 101] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit annoyed at the magic constants rather than computing these as needed, but I'm not sure that it's better.
use super::{chi_squared_fit, chi_squared_independence, BinDistribution, Histogram}; | ||
use std::collections::BTreeMap; | ||
|
||
const SAMPLES_2D: [Option<[usize; 2]>; 6] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment here about why the values were chosen please.
let ideal_first = BinDistribution::from_weights(vec![1.; 2]); | ||
let ideal_second = BinDistribution::from_weights(vec![1.; 2]); | ||
let chi_squared = chi_squared_independence(&histogram, &[ideal_first, ideal_second]); | ||
assert!((chi_squared - 10. / 3.).abs() < 1e-7); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This very much needs a comment.
// Uniform distribution on 5 bins 0..5 | ||
let ideal = BinDistribution::from_weights(vec![1.; 5]); | ||
let chi_squared = chi_squared_fit(&histogram, &ideal); | ||
assert!((chi_squared - 3.5).abs() < 1e-7); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment please.
fn histogram_project_two_duplicate() { | ||
let histogram: Histogram<2> = SAMPLES_2D.into_iter().collect(); | ||
|
||
// Verify that diagonal projections work how one would expect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't clear.
// Critical Values // | ||
//-----------------// | ||
|
||
/// Critical values of the chi-squared distribution for ascending degrees of freedom at an alpha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a line in here about what a critical value is, and maybe a doc test about how it would be used.
where | ||
T: Binned<N> + WithBinDistributions<N> + Copy, | ||
{ | ||
let rng = StdRng::from_entropy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be using unseeded RNG in our automated tests unfortunately.
|
||
/// A discretized distribution for the boundary of a [`Sphere`]. | ||
#[derive(Clone, Copy)] | ||
pub struct SphereBoundaryBins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just be able to make these fields pub right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree on the feature flag here: I think that InteriorOf / BoundaryOf can stay unflagged, but the statistical tests should be flagged.
As for the content of this PR, the math makes sense to me, and I think it's valuable to have tests for this. I think this could use a bit more care / explanation on how we expose this (these types are pub, but their actual usage is all in the private test). A module level doc test would be nice. Alternatively, we could decide that this is all private test code and cfg[test] the whole lot.
Indexing into an array is a bit dicey for gathering critical values / performing tests: perhaps we can write something a bit safer? Non-blocking though: this doesn't need to be super robust.
Unseeded RNG in tests is also a blocker, even if I expect spurious failures to be rare.
Regarding spurious failures: at least the false-negative rate could be calculated exactly in this case 😛 |
Objective
In #12484 the question arose of how we would actually go about testing the point-sampling methods introduced. In this PR, we introduce statistical tools for assessing the quality of spatial distributions in general and, in particular, of the
ShapeSample
implementations that presently exist.Background and approach
A uniform probability distribution is one where the probability density is proportional to the area — that is, for any given region, the probability of a sample being drawn from that region is equal to the proportion of the total area that region occupies.
It follows from this that, if one discretizes the sample space by partitioning it into labeled regions and assigning to each sample the label of the region it falls into, the discrete probability distribution sampled from the labels is a multinomial distribution with probabilities given by the proportions of the total area taken by each region of the partition.
Given, then, some probability distribution which is supposed to be uniform on some region, we can attempt to assess its uniformity by discretizing — as described above — and then performing statistical analysis of the resulting discrete distribution using Pearson's chi-squared test. The point is that, if the distribution exhibits some bias, it might be detected in the discrete distribution, which will fail to conform adequately to the associated multinomial density.
Solution
This branch contains a small library that supports this process with a few parts:
The preceding trait models the discretization process for arbitrary spatial distributions, but provides no metadata about what the associated multinomial densities should be; that is supported by the following additional trait:
Next, an
N
-dimensional histogram type is used to actually aggregate samples for the purposes of comparison:Finally, chi-squared analysis functions take these histograms (or their projections) as input and produce actual chi-squared values:
Presently, the actual testing implemented by this branch includes
Binned
implementations for the interiors and boundaries ofCircle
andSphere
. Two wrapper types,InteriorOf<T>
andBoundaryOf<T>
have been introduced for implementors ofShapeSample
, with the purpose of allowing the constituent sampling methods to be used directly asDistribution
s. This adds modularity; the library itself operates also at the level ofDistribution
s.Changelog
shape_sampling.rs
into a newsampling
submodule ofbevy_math
that holds all of therand
dependencies.InteriorOf<T>
andBoundaryOf<T>
allow conversion ofShapeSample
implementors intoDistribution
s.Discussion
Caveat emptor
The statistical tests in
sampling/statistical_tests/impls.rs
are marked#[ignore]
so that they do not run in CI testing. They must never, ever, ever run in CI testing. The purpose of these statistical tests is that they reliably fail when something is wrong — not that they always succeed when everything is fine.Presently, the alpha-level of each individual test is .001, meaning that each constituent check fails 1/1000th of the time; with the current volume of tests, this means that about 1% of the time, a failure would occur even if everything was perfect.
On the other hand, chi-squared error has the property that it grows with sample size for mismatched distributions, while remaining constant for matched ones. That is to say: statistical biases in the output should lead to the tests failing quite reliably, meaning they do not need to be run particularly often. We can use very large sample sizes to ensure this if need be.
Personally, I am not sure what the best way of using these tests would be other than running them manually. Presently, this can be done as follows:
What?
I'm sure this looks like building a death ray to kill an ant. In a sense, it is. Frankly, the reason that I made this isn't because I wanted to (not that I didn't enjoy myself), but really that I couldn't think of any other way to externally assess the quality of our sampling code that was actually meaningful in any way. For example, using a fixed-seed RNG and comparing output to some known values doesn't really demonstrate anything (and, in fact, breaks spuriously when the code is refactored).