diff --git a/CHANGELOG.md b/CHANGELOG.md index c291b83..1d1acee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `UncertainError::UnsupportedNode` - returned by `ComputationNode` evaluation methods when a graph node isn't evaluable in the requested domain (e.g. a boolean `BinaryOp`, for which no operation is defined). +- `ComputationNode::constant` - creates a leaf whose value is structurally known to be + constant, for callers building computation graphs directly with the low-level API. + This is the only way to make a node eligible for the optimizer's identity + elimination/constant folding passes; `Uncertain::point` now uses it internally. + +### Fixed + +- `GraphOptimizer`'s constant-folding and identity-elimination passes (`x + 0`, `x * 1`, + `x * 0`, etc.) no longer decide whether a node is constant by sampling it 3-4 times and + comparing the results. That approach could silently miscompile a low-entropy + distribution into a constant purely by chance (e.g. `bernoulli(0.99)` had roughly a + 96% chance of sampling the same value 4 times in a row) and fold it away, changing the + distribution the optimized graph represents. Constancy is now decided structurally: a + node is constant only if it was built via the new `ComputationNode::constant` + (transitively, `Uncertain::point`) or is itself a folded result of already-constant + operands — never by observing sampled behavior. ### Changed diff --git a/crap_baseline.json b/crap_baseline.json index 968f819..4ad545a 100644 --- a/crap_baseline.json +++ b/crap_baseline.json @@ -365,7 +365,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::binary_op", - "line": 321, + "line": 351, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -374,7 +374,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::compute_complexity", - "line": 405, + "line": 435, "cyclomatic": 5.0, "coverage": 100.0, "crap": 5.0, @@ -383,7 +383,16 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::conditional", - "line": 346, + "line": 376, + "cyclomatic": 1.0, + "coverage": 100.0, + "crap": 1.0, + "crate": "uncertain-rs" + }, + { + "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", + "function": "ComputationNode::constant", + "line": 337, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -392,7 +401,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::depth", - "line": 377, + "line": 407, "cyclomatic": 5.0, "coverage": 100.0, "crap": 5.0, @@ -401,7 +410,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::evaluate", - "line": 191, + "line": 199, "cyclomatic": 9.0, "coverage": 100.0, "crap": 9.0, @@ -410,7 +419,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::evaluate_arithmetic", - "line": 240, + "line": 248, "cyclomatic": 13.0, "coverage": 100.0, "crap": 13.0, @@ -419,7 +428,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::evaluate_bool", - "line": 480, + "line": 510, "cyclomatic": 11.0, "coverage": 100.0, "crap": 11.0, @@ -428,7 +437,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::evaluate_fresh", - "line": 299, + "line": 307, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -437,7 +446,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::has_conditionals", - "line": 392, + "line": 422, "cyclomatic": 6.0, "coverage": 100.0, "crap": 6.0, @@ -446,7 +455,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::hash_structure", - "line": 435, + "line": 465, "cyclomatic": 5.0, "coverage": 55.172413793103445, "crap": 7.252039854032556, @@ -455,7 +464,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::leaf", - "line": 309, + "line": 320, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -464,7 +473,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::map", - "line": 334, + "line": 364, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -473,7 +482,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::node_count", - "line": 360, + "line": 390, "cyclomatic": 5.0, "coverage": 100.0, "crap": 5.0, @@ -482,7 +491,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "ComputationNode::structural_hash", - "line": 426, + "line": 456, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -491,43 +500,43 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::check_addition_identities", - "line": 674, - "cyclomatic": 5.0, + "line": 704, + "cyclomatic": 3.0, "coverage": 100.0, - "crap": 5.0, + "crap": 3.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::check_division_identities", - "line": 798, - "cyclomatic": 3.0, + "line": 759, + "cyclomatic": 2.0, "coverage": 100.0, - "crap": 3.0, + "crap": 2.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::check_multiplication_identities", - "line": 733, - "cyclomatic": 9.0, + "line": 735, + "cyclomatic": 5.0, "coverage": 100.0, - "crap": 9.0, + "crap": 5.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::check_subtraction_identities", - "line": 710, - "cyclomatic": 3.0, + "line": 721, + "cyclomatic": 2.0, "coverage": 100.0, - "crap": 3.0, + "crap": 2.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::constant_folding", - "line": 912, + "line": 871, "cyclomatic": 5.0, "coverage": 100.0, "crap": 5.0, @@ -536,16 +545,16 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::constant_folding_binary_op", - "line": 935, - "cyclomatic": 8.0, - "coverage": 79.3103448275862, - "crap": 8.56681290745828, + "line": 894, + "cyclomatic": 6.0, + "coverage": 100.0, + "crap": 6.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::constant_folding_bool", - "line": 1042, + "line": 995, "cyclomatic": 4.0, "coverage": 40.0, "crap": 7.4559999999999995, @@ -554,43 +563,43 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::constant_folding_bool_conditional", - "line": 1084, - "cyclomatic": 4.0, + "line": 1035, + "cyclomatic": 3.0, "coverage": 0.0, - "crap": 20.0, + "crap": 12.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::constant_folding_bool_unary_op", - "line": 1057, - "cyclomatic": 5.0, + "line": 1010, + "cyclomatic": 4.0, "coverage": 0.0, - "crap": 30.0, + "crap": 20.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::constant_folding_conditional", - "line": 1008, - "cyclomatic": 4.0, - "coverage": 76.0, - "crap": 4.221184, + "line": 963, + "cyclomatic": 3.0, + "coverage": 100.0, + "crap": 3.0, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::constant_folding_unary_op", - "line": 978, - "cyclomatic": 5.0, - "coverage": 71.42857142857143, - "crap": 5.5830903790087465, + "line": 934, + "cyclomatic": 4.0, + "coverage": 70.0, + "crap": 4.432, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::default", - "line": 1142, + "line": 1064, "cyclomatic": 1.0, "coverage": 0.0, "crap": 2.0, @@ -599,7 +608,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::eliminate_common_subexpressions", - "line": 544, + "line": 574, "cyclomatic": 7.0, "coverage": 61.36363636363637, "crap": 9.82608236288505, @@ -608,7 +617,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::eliminate_identity_operations", - "line": 608, + "line": 638, "cyclomatic": 5.0, "coverage": 62.5, "crap": 6.318359375, @@ -617,16 +626,16 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::eliminate_identity_operations_binary", - "line": 631, + "line": 661, "cyclomatic": 9.0, - "coverage": 89.28571428571429, - "crap": 9.099626457725947, + "coverage": 96.42857142857143, + "crap": 9.003689868804665, "crate": "uncertain-rs" }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::eliminate_identity_operations_bool", - "line": 856, + "line": 808, "cyclomatic": 4.0, "coverage": 0.0, "crap": 20.0, @@ -635,7 +644,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::eliminate_identity_operations_conditional", - "line": 836, + "line": 788, "cyclomatic": 1.0, "coverage": 0.0, "crap": 2.0, @@ -644,34 +653,16 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::eliminate_identity_operations_unary", - "line": 821, + "line": 773, "cyclomatic": 1.0, "coverage": 0.0, "crap": 2.0, "crate": "uncertain-rs" }, - { - "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", - "function": "GraphOptimizer::is_constant", - "line": 1114, - "cyclomatic": 3.0, - "coverage": 100.0, - "crap": 3.0, - "crate": "uncertain-rs" - }, - { - "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", - "function": "GraphOptimizer::is_constant_bool", - "line": 1129, - "cyclomatic": 3.0, - "coverage": 100.0, - "crap": 3.0, - "crate": "uncertain-rs" - }, { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::is_constant_one", - "line": 898, + "line": 857, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -680,7 +671,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::is_constant_zero", - "line": 884, + "line": 841, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -689,7 +680,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::new", - "line": 526, + "line": 556, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -698,7 +689,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphOptimizer::optimize", - "line": 534, + "line": 564, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -707,7 +698,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphProfiler::default", - "line": 1344, + "line": 1266, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -716,7 +707,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphProfiler::get_stats", - "line": 1300, + "line": 1222, "cyclomatic": 3.0, "coverage": 95.45454545454545, "crap": 3.000845229151014, @@ -725,7 +716,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphProfiler::new", - "line": 1275, + "line": 1197, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -734,7 +725,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphProfiler::print_report", - "line": 1327, + "line": 1249, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -743,7 +734,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphProfiler::profile_execution", - "line": 1282, + "line": 1204, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -752,7 +743,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphVisualizer::add_node_to_dot", - "line": 1164, + "line": 1086, "cyclomatic": 9.0, "coverage": 93.02325581395348, "crap": 9.02750701196121, @@ -761,7 +752,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphVisualizer::print_tree", - "line": 1221, + "line": 1143, "cyclomatic": 9.0, "coverage": 47.22222222222222, "crap": 20.907986111111114, @@ -770,7 +761,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/computation.rs", "function": "GraphVisualizer::to_dot", - "line": 1153, + "line": 1075, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -878,7 +869,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::bernoulli", - "line": 408, + "line": 421, "cyclomatic": 2.0, "coverage": 100.0, "crap": 2.0, @@ -887,7 +878,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::beta", - "line": 349, + "line": 362, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -896,7 +887,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::binomial", - "line": 443, + "line": 456, "cyclomatic": 2.0, "coverage": 100.0, "crap": 2.0, @@ -905,7 +896,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::categorical", - "line": 205, + "line": 218, "cyclomatic": 3.0, "coverage": 94.44444444444444, "crap": 3.001543209876543, @@ -914,7 +905,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::empirical", - "line": 169, + "line": 182, "cyclomatic": 2.0, "coverage": 100.0, "crap": 2.0, @@ -923,7 +914,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::exponential", - "line": 304, + "line": 317, "cyclomatic": 2.0, "coverage": 100.0, "crap": 2.0, @@ -932,7 +923,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::gamma", - "line": 373, + "line": 386, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -941,7 +932,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::gamma_unchecked", - "line": 379, + "line": 392, "cyclomatic": 2.0, "coverage": 100.0, "crap": 2.0, @@ -950,7 +941,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::geometric", - "line": 508, + "line": 521, "cyclomatic": 2.0, "coverage": 100.0, "crap": 2.0, @@ -959,7 +950,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::log_normal", - "line": 326, + "line": 339, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -968,7 +959,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::mixture", - "line": 112, + "line": 125, "cyclomatic": 6.0, "coverage": 96.96969696969697, "crap": 6.001001753067869, @@ -977,7 +968,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::normal", - "line": 248, + "line": 261, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -986,7 +977,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::normal_unchecked", - "line": 254, + "line": 267, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -995,7 +986,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::point", - "line": 87, + "line": 89, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -1004,7 +995,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::poisson", - "line": 471, + "line": 484, "cyclomatic": 4.0, "coverage": 100.0, "crap": 4.0, @@ -1013,7 +1004,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "Uncertain::uniform", - "line": 276, + "line": 289, "cyclomatic": 4.0, "coverage": 100.0, "crap": 4.0, @@ -1022,7 +1013,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "validate_finite", - "line": 13, + "line": 15, "cyclomatic": 2.0, "coverage": 100.0, "crap": 2.0, @@ -1031,7 +1022,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "validate_left_open_unit_interval", - "line": 60, + "line": 62, "cyclomatic": 4.0, "coverage": 100.0, "crap": 4.0, @@ -1040,7 +1031,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "validate_non_negative", - "line": 34, + "line": 36, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -1049,7 +1040,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "validate_positive", - "line": 21, + "line": 23, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -1058,7 +1049,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/distributions.rs", "function": "validate_unit_interval", - "line": 47, + "line": 49, "cyclomatic": 3.0, "coverage": 100.0, "crap": 3.0, @@ -2246,7 +2237,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::eq_value", - "line": 470, + "line": 471, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2255,7 +2246,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::filter", - "line": 193, + "line": 194, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2264,7 +2255,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::flat_map", - "line": 170, + "line": 171, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2273,7 +2264,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::fmt", - "line": 487, + "line": 488, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2282,7 +2273,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::ge", - "line": 453, + "line": 454, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2291,7 +2282,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::greater_than", - "line": 409, + "line": 410, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2300,7 +2291,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::gt", - "line": 439, + "line": 440, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2309,7 +2300,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::id", - "line": 97, + "line": 98, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2318,7 +2309,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::le", - "line": 460, + "line": 461, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2327,7 +2318,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::less_than", - "line": 396, + "line": 397, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2336,7 +2327,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::lt", - "line": 446, + "line": 447, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2345,7 +2336,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::map", - "line": 151, + "line": 152, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2354,7 +2345,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::ne_value", - "line": 477, + "line": 478, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2372,7 +2363,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::sample", - "line": 112, + "line": 113, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2381,7 +2372,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::sample_with", - "line": 135, + "line": 136, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2390,7 +2381,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::samples", - "line": 218, + "line": 219, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2399,7 +2390,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::take_samples", - "line": 232, + "line": 233, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2408,7 +2399,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::take_samples_cached", - "line": 350, + "line": 351, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2417,7 +2408,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::take_samples_cached_par", - "line": 382, + "line": 383, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2426,7 +2417,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::take_samples_par", - "line": 290, + "line": 291, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2435,7 +2426,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::take_samples_with", - "line": 256, + "line": 257, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2444,7 +2435,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::take_samples_with_par", - "line": 320, + "line": 321, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, @@ -2453,7 +2444,7 @@ { "file": "/Users/mnkn/GitHub/uncertain-rs/src/uncertain.rs", "function": "Uncertain::with_node", - "line": 73, + "line": 74, "cyclomatic": 1.0, "coverage": 100.0, "crap": 1.0, diff --git a/examples/optimizations/graph_optimization.rs b/examples/optimizations/graph_optimization.rs index fbfb020..8f0a1ed 100644 --- a/examples/optimizations/graph_optimization.rs +++ b/examples/optimizations/graph_optimization.rs @@ -36,8 +36,8 @@ fn main() { let x_node = ComputationNode::leaf(|| 2.0); let y_node = ComputationNode::leaf(|| 3.0); let z_node = ComputationNode::leaf(|| 1.0); - let zero_node = ComputationNode::leaf(|| 0.0); - let one_node = ComputationNode::leaf(|| 1.0); + let zero_node = ComputationNode::constant(0.0); + let one_node = ComputationNode::constant(1.0); let sum_node = ComputationNode::binary_op(x_node.clone(), y_node.clone(), BinaryOperation::Add); diff --git a/specs/07-sound-constant-folding.md b/specs/07-sound-constant-folding.md index 23db184..dcd9a66 100644 --- a/specs/07-sound-constant-folding.md +++ b/specs/07-sound-constant-folding.md @@ -1,6 +1,6 @@ # Spec 07 — Sound Constant Folding -**Status:** Pending | **Effort:** Medium | **Module:** `src/computation.rs` (GraphOptimizer) +**Status:** Implemented | **Effort:** Medium | **Module:** `src/computation.rs` (GraphOptimizer) ## Context @@ -40,3 +40,54 @@ user's model. An optimizer must never alter distributional semantics. unoptimized with the same seed (Spec 04), **then** sampled outputs are identical. - **Given** the source of `GraphOptimizer`, **when** grepped, **then** no `sample()` call participates in any `is_constant*` decision. + +## Implementation notes + +- **`constant_value: Option`, not just a bool.** `ComputationNode::Leaf` gained a + `constant_value: Option` field (not merely an `is_constant: bool`) precisely so the + last acceptance test holds *literally*: `is_constant_zero`/`is_constant_one`/the + constant-folding match arms compare `constant_value` directly and never call `sample` + at all — not even once after confirming constancy. A bool-flag version was tried first + and still called `sample()` once to read the value for the zero/one comparison; storing + the value directly removes that call entirely, at the cost of one extra (small, + `Option`) field per leaf. +- **The only way to get `constant_value: Some(_)`** is `ComputationNode::constant(value)` + (new `pub` constructor) or the optimizer's own folded results, which are always + rebuilt via `ComputationNode::constant` too — so a folded subexpression is eligible for + further folding one level up, exactly like a literal. `ComputationNode::leaf(...)` + (the pre-existing general constructor) always sets `constant_value: None`, including + for a closure that happens to be deterministic (e.g. always returns `0.0`) — constancy + is a structural fact about *how a node was built*, never an inference from behavior. +- **`Uncertain::point` now builds via `ComputationNode::constant`** instead of the + generic `Uncertain::new`, so every `point(v)` is structurally constant to the optimizer + without changing `point`'s public signature or behavior. +- **`GraphOptimizer` is not wired into `Uncertain`'s arithmetic operators** (`+`, `*`, + etc. in `src/operations/arithmetic.rs` build a graph via `Uncertain::with_node` and + never call `GraphOptimizer::optimize`) — discovered while implementing this spec, and + unchanged by it: `GraphOptimizer` remains an opt-in utility a caller invokes manually + (as `examples/optimizations/*.rs` already did). This affects how the acceptance tests + are exercised: the `bernoulli(0.99)`-style test uses a deterministic low-entropy + closure (`ComputationNode::leaf(|| if rand::random::() < 0.99 {1.0} else {0.0})`) + and asserts `is_constant_zero`/`is_constant_one` structurally rather than routing + through `Uncertain::bernoulli` + `probability_exceeds` end-to-end, since there's no + automatic optimization pass in that path to exercise. The `point(0.0) + x` / + `point(2.0) * point(3.0)` / same-seed-equivalence tests do use the real public + `Uncertain` API (`Uncertain::point`, `Uncertain::normal`, `sample_with`), reaching + into the `pub(crate) node` field and calling `GraphOptimizer::new().optimize(...)` + manually, since that's the only way this functionality is invoked today. Whether to + wire optimization into the arithmetic operators automatically is a separate, unscoped + design decision — flagged here for Spec 08 (effective CSE) to pick up, since CSE has + the same "never automatically invoked" characteristic. +- **`examples/optimizations/graph_optimization.rs`** had its literal `0`/`1` operands + (`zero_node`/`one_node`) switched from `ComputationNode::leaf(...)` to + `ComputationNode::constant(...)` — with the old sampling-based check removed, the demo + would otherwise silently stop demonstrating identity elimination/constant folding + (the operations just wouldn't fire) while still printing the same correct final number + from evaluation. `examples/optimizations/common_subexpression_optimization.rs` only + exercises CSE (not identity/constant folding) and needed no change. +- Ten pre-existing functions' CRAP scores *improved* as a side effect (mostly the + `check_*_identities` helpers, simplified by moving the `Leaf`-pattern-match out of the + call sites and into `is_constant_zero`/`is_constant_one` themselves); `crap_baseline.json` + regenerated to capture this, plus the new `ComputationNode::constant` function and the + removal of the old sample-based `is_constant`/`is_constant_bool` helpers (no longer + needed now that callers pattern-match `constant_value` directly). diff --git a/specs/README.md b/specs/README.md index f5c4784..d483370 100644 --- a/specs/README.md +++ b/specs/README.md @@ -15,7 +15,7 @@ a spec is closed only when every acceptance test passes and `just dev` is green. | 04 | [Seedable RNG & reproducibility](04-seedable-rng.md) | P0 | High | no | Implemented | | 05 | [Correct sampling via rand_distr](05-rand-distr-sampling.md) | P0 | Medium | no | Implemented | | 06 | [Total graph evaluation](06-total-graph-evaluation.md) | P0 | Medium | **yes** | Implemented | -| 07 | [Sound constant folding](07-sound-constant-folding.md) | P0 | Medium | no | Pending | +| 07 | [Sound constant folding](07-sound-constant-folding.md) | P0 | Medium | no | Implemented | | 08 | [Effective CSE](08-effective-cse.md) | P1 | Medium | no | Pending | | 09 | [Consistent variance policy](09-variance-policy.md) | P1 | Low | behavioral | Pending | | 10 | [Bounded caches, no NaN paths](10-bounded-caches.md) | P1 | Medium | no | Partially implemented | @@ -58,6 +58,12 @@ a spec is closed only when every acceptance test passes and `just dev` is green. count/quantile/confidence/bandwidth are genuine per-call parameters) — see 18's spec for the full per-method breakdown.)_ 4. **07 → 08** — optimizer correctness before optimizer effectiveness. + _(07 implemented; constancy decided via a `constant_value: Option` field on + `Leaf` — set only by the new `ComputationNode::constant`/`Uncertain::point` — rather + than the old sample-3x-and-compare check, which could silently fold a low-entropy + distribution (e.g. `bernoulli(0.99)`) into a constant. Discovered along the way: + `GraphOptimizer` isn't wired into `Uncertain`'s arithmetic operators at all — it's + an opt-in utility invoked manually, unchanged by this spec but relevant to 08.)_ 5. **09, 10, 11, 14, 15, 17, 19** in any order. 6. **12, 13, 16** — feature/polish tail. diff --git a/src/computation.rs b/src/computation.rs index 2542b08..d0b29c1 100644 --- a/src/computation.rs +++ b/src/computation.rs @@ -144,6 +144,14 @@ pub enum ComputationNode { Leaf { id: uuid::Uuid, sample: Arc T + Send + Sync>, + /// The value this leaf is structurally known to always produce, if any. + /// `Some` only for leaves built via [`ComputationNode::constant`] (and, + /// transitively, `Uncertain::point` and the optimizer's own folded results) — + /// storing the value directly (rather than re-sampling to check it) means the + /// optimizer's constancy checks never call `sample` at all. Never set by + /// sampling and comparing — a low-entropy distribution (e.g. `bernoulli(0.99)`) + /// could return equal samples by chance and be silently, incorrectly folded. + constant_value: Option, }, /// Binary operation node for combining two uncertain values @@ -190,7 +198,7 @@ where /// which handles conditionals, or `evaluate_bool` for boolean graphs). pub fn evaluate(&self, context: &mut SampleContext) -> Result { match self { - ComputationNode::Leaf { id, sample } => { + ComputationNode::Leaf { id, sample, .. } => { // Check if we already have a memoized value for this node if let Some(cached) = context.get_value::(id) { Ok(cached) @@ -242,7 +250,7 @@ where T: Arithmetic, { match self { - ComputationNode::Leaf { id, sample } => { + ComputationNode::Leaf { id, sample, .. } => { if let Some(cached) = context.get_value::(id) { Ok(cached) } else { @@ -305,7 +313,10 @@ where .expect("graphs built via the public Uncertain API are always well-formed") } - /// Creates a new leaf node + /// Creates a new leaf node from an arbitrary sampling function + /// + /// The result is never treated as structurally constant by the optimizer — use + /// [`ComputationNode::constant`] for a leaf whose value is known not to vary. pub fn leaf(sample: F) -> Self where F: Fn() -> T + Send + Sync + 'static, @@ -313,6 +324,25 @@ where ComputationNode::Leaf { id: uuid::Uuid::new_v4(), sample: Arc::new(sample), + constant_value: None, + } + } + + /// Creates a leaf node whose value is structurally known to be constant + /// + /// This is the only way to mark a node as constant for the optimizer's identity + /// elimination and constant folding passes — deciding constancy by sampling and + /// comparing is unsound (a low-entropy distribution can return equal samples by + /// chance) and is never done. + pub fn constant(value: T) -> Self + where + T: Clone + Send + Sync + 'static, + { + let stored = value.clone(); + ComputationNode::Leaf { + id: uuid::Uuid::new_v4(), + sample: Arc::new(move || value.clone()), + constant_value: Some(stored), } } @@ -479,7 +509,7 @@ impl ComputationNode { /// operations, and `bool` doesn't implement `Arithmetic`). pub fn evaluate_bool(&self, context: &mut SampleContext) -> Result { match self { - ComputationNode::Leaf { id, sample } => { + ComputationNode::Leaf { id, sample, .. } => { if let Some(cached) = context.get_value::(id) { Ok(cached) } else { @@ -678,30 +708,11 @@ impl GraphOptimizer { where T: Shareable + Arithmetic + PartialEq + Clone, { - match (left, right) { - // x + 0 = x - ( - left, - ComputationNode::Leaf { - sample: right_sample, - .. - }, - ) => { - if Self::is_constant_zero(right_sample) { - return Some(left.clone()); - } - } - // 0 + x = x - ( - ComputationNode::Leaf { - sample: left_sample, - .. - }, - right, - ) if Self::is_constant_zero(left_sample) => { - return Some(right.clone()); - } - _ => {} + if Self::is_constant_zero(right) { + return Some(left.clone()); + } + if Self::is_constant_zero(left) { + return Some(right.clone()); } None } @@ -714,16 +725,7 @@ impl GraphOptimizer { where T: Shareable + Arithmetic + PartialEq + Clone, { - // x - 0 = x - if let ( - left, - ComputationNode::Leaf { - sample: right_sample, - .. - }, - ) = (left, right) - && Self::is_constant_zero(right_sample) - { + if Self::is_constant_zero(right) { return Some(left.clone()); } None @@ -737,58 +739,17 @@ impl GraphOptimizer { where T: Shareable + Arithmetic + PartialEq + Clone, { - // Check for zero multiplication first (x * 0 = 0, 0 * x = 0) - match (left, right) { - // x * 0 = 0 - ( - _left, - ComputationNode::Leaf { - sample: right_sample, - .. - }, - ) => { - if Self::is_constant_zero(right_sample) { - return Some(ComputationNode::leaf(|| T::zero())); - } - } - // 0 * x = 0 - ( - ComputationNode::Leaf { - sample: left_sample, - .. - }, - _right, - ) if Self::is_constant_zero(left_sample) => { - return Some(ComputationNode::leaf(|| T::zero())); - } - _ => {} + // x * 0 = 0, 0 * x = 0 + if Self::is_constant_zero(right) || Self::is_constant_zero(left) { + return Some(ComputationNode::constant(T::zero())); } - // Check for identity multiplication (x * 1 = x, 1 * x = x) - match (left, right) { - // x * 1 = x - ( - left, - ComputationNode::Leaf { - sample: right_sample, - .. - }, - ) => { - if Self::is_constant_one(right_sample) { - return Some(left.clone()); - } - } - // 1 * x = x - ( - ComputationNode::Leaf { - sample: left_sample, - .. - }, - right, - ) if Self::is_constant_one(left_sample) => { - return Some(right.clone()); - } - _ => {} + // x * 1 = x, 1 * x = x + if Self::is_constant_one(right) { + return Some(left.clone()); + } + if Self::is_constant_one(left) { + return Some(right.clone()); } None @@ -802,16 +763,7 @@ impl GraphOptimizer { where T: Shareable + Arithmetic + PartialEq + Clone, { - // x / 1 = x - if let ( - left, - ComputationNode::Leaf { - sample: right_sample, - .. - }, - ) = (left, right) - && Self::is_constant_one(right_sample) - { + if Self::is_constant_one(right) { return Some(left.clone()); } None @@ -880,32 +832,39 @@ impl GraphOptimizer { } } - /// Helper function to check if a sampling function returns zero - fn is_constant_zero(sample_fn: &Arc T + Send + Sync>) -> bool + /// Checks whether a node is structurally constant and equal to zero + /// + /// Never decided by sampling: a low-entropy distribution could return equal + /// samples by chance and be silently, incorrectly folded. Only leaves built via + /// [`ComputationNode::constant`] carry a `constant_value`; it's compared directly + /// with no call to `sample` at all. + fn is_constant_zero(node: &ComputationNode) -> bool where - T: PartialEq + Clone + Arithmetic, + T: PartialEq + Arithmetic, { - // Sample a few times to check if it's consistently zero - for _ in 0..3 { - if sample_fn() != T::zero() { - return false; - } + match node { + ComputationNode::Leaf { + constant_value: Some(value), + .. + } => *value == T::zero(), + _ => false, } - true } - /// Helper function to check if a sampling function returns one - fn is_constant_one(sample_fn: &Arc T + Send + Sync>) -> bool + /// Checks whether a node is structurally constant and equal to one + /// + /// See [`GraphOptimizer::is_constant_zero`] for why this is structural, not sampled. + fn is_constant_one(node: &ComputationNode) -> bool where - T: PartialEq + Clone + Arithmetic, + T: PartialEq + Arithmetic, { - // Sample a few times to check if it's consistently one - for _ in 0..3 { - if sample_fn() != T::one() { - return false; - } + match node { + ComputationNode::Leaf { + constant_value: Some(value), + .. + } => *value == T::one(), + _ => false, } - true } /// Performs constant folding for compile-time evaluation of constant expressions @@ -945,26 +904,23 @@ impl GraphOptimizer { if let ( ComputationNode::Leaf { - sample: left_sample, + constant_value: Some(left_val), .. }, ComputationNode::Leaf { - sample: right_sample, + constant_value: Some(right_val), .. }, ) = (&left_opt, &right_opt) - && Self::is_constant(left_sample) - && Self::is_constant(right_sample) { - let left_val = left_sample(); - let right_val = right_sample(); + let (left_val, right_val) = (left_val.clone(), right_val.clone()); let result = match operation { BinaryOperation::Add => left_val + right_val, BinaryOperation::Sub => left_val - right_val, BinaryOperation::Mul => left_val * right_val, BinaryOperation::Div => left_val / right_val, }; - return ComputationNode::leaf(move || result.clone()); + return ComputationNode::constant(result); } ComputationNode::BinaryOp { @@ -985,17 +941,16 @@ impl GraphOptimizer { let operand_opt = Self::constant_folding(operand); if let ComputationNode::Leaf { - sample: operand_sample, + constant_value: Some(operand_val), .. } = &operand_opt - && Self::is_constant(operand_sample) { - let operand_val = operand_sample(); + let operand_val = operand_val.clone(); let result = match operation { UnaryOperation::Map(func) => func(operand_val), UnaryOperation::Filter(_) => operand_val, // Filter doesn't change the value }; - return ComputationNode::leaf(move || result.clone()); + return ComputationNode::constant(result); } ComputationNode::UnaryOp { @@ -1019,13 +974,11 @@ impl GraphOptimizer { // Check if condition is constant if let ComputationNode::Leaf { - sample: condition_sample, + constant_value: Some(condition_val), .. } = &condition_opt - && Self::is_constant_bool(condition_sample) { - let condition_val = condition_sample(); - if condition_val { + if *condition_val { return if_true_opt; } return if_false_opt; @@ -1061,17 +1014,15 @@ impl GraphOptimizer { let operand_opt = Self::constant_folding_bool(operand); if let ComputationNode::Leaf { - sample: operand_sample, + constant_value: Some(operand_val), .. } = &operand_opt - && Self::is_constant_bool(operand_sample) { - let operand_val = operand_sample(); let result = match operation { - UnaryOperation::Map(func) => func(operand_val), - UnaryOperation::Filter(_) => operand_val, // Filter doesn't change the value + UnaryOperation::Map(func) => func(*operand_val), + UnaryOperation::Filter(_) => *operand_val, // Filter doesn't change the value }; - return ComputationNode::leaf(move || result); + return ComputationNode::constant(result); } ComputationNode::UnaryOp { @@ -1091,13 +1042,11 @@ impl GraphOptimizer { let if_false_opt = Self::constant_folding_bool(if_false); if let ComputationNode::Leaf { - sample: condition_sample, + constant_value: Some(condition_val), .. } = &condition_opt - && Self::is_constant_bool(condition_sample) { - let condition_val = condition_sample(); - if condition_val { + if *condition_val { return if_true_opt; } return if_false_opt; @@ -1109,33 +1058,6 @@ impl GraphOptimizer { if_false: Box::new(if_false_opt), } } - - /// Helper function to check if a sampling function returns a constant value - fn is_constant(sample_fn: &Arc T + Send + Sync>) -> bool - where - T: PartialEq + Clone, - { - // Sample a few times to check if it's consistently the same value - let first_sample = sample_fn(); - for _ in 0..3 { - if sample_fn() != first_sample { - return false; - } - } - true - } - - /// Helper function to check if a boolean sampling function returns a constant value - fn is_constant_bool(sample_fn: &Arc bool + Send + Sync>) -> bool { - // Sample a few times to check if it's consistently the same value - let first_sample = sample_fn(); - for _ in 0..3 { - if sample_fn() != first_sample { - return false; - } - } - true - } } impl Default for GraphOptimizer { @@ -1405,6 +1327,7 @@ mod tests { let leaf = ComputationNode::Leaf { id: leaf_id, sample: Arc::new(rand::random::), + constant_value: None, }; // Evaluate twice with the same context @@ -1692,7 +1615,7 @@ mod tests { fn test_identity_operation_elimination() { // Test x + 0 = x let x = ComputationNode::leaf(|| 5.0); - let zero = ComputationNode::leaf(|| 0.0); + let zero = ComputationNode::constant(0.0); let add_zero = ComputationNode::binary_op(x.clone(), zero, BinaryOperation::Add); let optimized = GraphOptimizer::eliminate_identity_operations(add_zero); @@ -1700,7 +1623,7 @@ mod tests { assert!((result - 5.0).abs() < f64::EPSILON); // Test x * 1 = x - let one = ComputationNode::leaf(|| 1.0); + let one = ComputationNode::constant(1.0); let mul_one = ComputationNode::binary_op(x.clone(), one, BinaryOperation::Mul); let optimized = GraphOptimizer::eliminate_identity_operations(mul_one); @@ -1708,7 +1631,7 @@ mod tests { assert!((result - 5.0).abs() < f64::EPSILON); // Test x - 0 = x - let zero2 = ComputationNode::leaf(|| 0.0); + let zero2 = ComputationNode::constant(0.0); let sub_zero = ComputationNode::binary_op(x.clone(), zero2, BinaryOperation::Sub); let optimized = GraphOptimizer::eliminate_identity_operations(sub_zero); @@ -1716,7 +1639,7 @@ mod tests { assert!((result - 5.0).abs() < f64::EPSILON); // Test x / 1 = x - let one2 = ComputationNode::leaf(|| 1.0); + let one2 = ComputationNode::constant(1.0); let div_one = ComputationNode::binary_op(x.clone(), one2, BinaryOperation::Div); let optimized = GraphOptimizer::eliminate_identity_operations(div_one); @@ -1724,7 +1647,7 @@ mod tests { assert!((result - 5.0).abs() < f64::EPSILON); // Test x * 0 = 0 - let zero3 = ComputationNode::leaf(|| 0.0); + let zero3 = ComputationNode::constant(0.0); let mul_zero = ComputationNode::binary_op(x.clone(), zero3, BinaryOperation::Mul); let optimized = GraphOptimizer::eliminate_identity_operations(mul_zero); @@ -1735,8 +1658,8 @@ mod tests { #[test] fn test_constant_folding() { // Test constant addition: 2 + 3 = 5 - let two = ComputationNode::leaf(|| 2.0); - let three = ComputationNode::leaf(|| 3.0); + let two = ComputationNode::constant(2.0); + let three = ComputationNode::constant(3.0); let add_const = ComputationNode::binary_op(two, three, BinaryOperation::Add); let optimized = GraphOptimizer::constant_folding(add_const); @@ -1744,8 +1667,8 @@ mod tests { assert!((result - 5.0).abs() < f64::EPSILON); // Test constant multiplication: 4 * 5 = 20 - let four = ComputationNode::leaf(|| 4.0); - let five = ComputationNode::leaf(|| 5.0); + let four = ComputationNode::constant(4.0); + let five = ComputationNode::constant(5.0); let mul_const = ComputationNode::binary_op(four, five, BinaryOperation::Mul); let optimized = GraphOptimizer::constant_folding(mul_const); @@ -1753,8 +1676,8 @@ mod tests { assert!((result - 20.0).abs() < f64::EPSILON); // Test constant division: 10 / 2 = 5 - let ten = ComputationNode::leaf(|| 10.0); - let two_div = ComputationNode::leaf(|| 2.0); + let ten = ComputationNode::constant(10.0); + let two_div = ComputationNode::constant(2.0); let div_const = ComputationNode::binary_op(ten, two_div, BinaryOperation::Div); let optimized = GraphOptimizer::constant_folding(div_const); @@ -1762,8 +1685,8 @@ mod tests { assert!((result - 5.0).abs() < f64::EPSILON); // Test constant subtraction: 8 - 3 = 5 - let eight = ComputationNode::leaf(|| 8.0); - let three_sub = ComputationNode::leaf(|| 3.0); + let eight = ComputationNode::constant(8.0); + let three_sub = ComputationNode::constant(3.0); let sub_const = ComputationNode::binary_op(eight, three_sub, BinaryOperation::Sub); let optimized = GraphOptimizer::constant_folding(sub_const); @@ -1774,9 +1697,9 @@ mod tests { #[test] fn test_constant_folding_conditional() { // Test constant condition: if true then 10 else 20 = 10 - let true_condition = ComputationNode::leaf(|| true); - let if_true = ComputationNode::leaf(|| 10.0); - let if_false = ComputationNode::leaf(|| 20.0); + let true_condition = ComputationNode::constant(true); + let if_true = ComputationNode::constant(10.0); + let if_false = ComputationNode::constant(20.0); let conditional = ComputationNode::conditional(true_condition, if_true, if_false); let optimized = GraphOptimizer::constant_folding(conditional); @@ -1784,9 +1707,9 @@ mod tests { assert!((result - 10.0).abs() < f64::EPSILON); // Test constant condition: if false then 10 else 20 = 20 - let false_condition = ComputationNode::leaf(|| false); - let if_true2 = ComputationNode::leaf(|| 10.0); - let if_false2 = ComputationNode::leaf(|| 20.0); + let false_condition = ComputationNode::constant(false); + let if_true2 = ComputationNode::constant(10.0); + let if_false2 = ComputationNode::constant(20.0); let conditional2 = ComputationNode::conditional(false_condition, if_true2, if_false2); let optimized = GraphOptimizer::constant_folding(conditional2); @@ -1797,7 +1720,7 @@ mod tests { #[test] fn test_constant_folding_unary() { // Test constant unary operation: map(|x| x * 2) on constant 5 = 10 - let five = ComputationNode::leaf(|| 5.0); + let five = ComputationNode::constant(5.0); let double = ComputationNode::map(five, |x| x * 2.0); let optimized = GraphOptimizer::constant_folding(double); @@ -1805,6 +1728,94 @@ mod tests { assert!((result - 10.0).abs() < f64::EPSILON); } + // Spec 07 acceptance tests: constant folding/identity elimination must decide + // constancy structurally, never by sampling. + + #[test] + fn test_low_entropy_distribution_never_treated_as_constant() { + // Simulates `bernoulli(0.99)` mapped to `f64`: P(1.0) = 0.99. Under the old + // sample-3x-and-compare semantics, ~96% of runs would see three consecutive + // 1.0s and be silently (and incorrectly) folded into a constant. Structural + // constancy never inspects sampled values at all, so this is now a + // deterministic guarantee rather than a statistical one -- true regardless + // of how "constant-looking" any given run's samples happen to be. + let low_entropy = ComputationNode::leaf(|| { + if rand::random::() < 0.99 { + 1.0 + } else { + 0.0 + } + }); + assert!(!GraphOptimizer::is_constant_zero(&low_entropy)); + assert!(!GraphOptimizer::is_constant_one(&low_entropy)); + + // Downstream: an identity check against it must not fire either. + let x = ComputationNode::leaf(|| 5.0); + assert!(GraphOptimizer::check_addition_identities(&x, &low_entropy).is_none()); + } + + #[test] + fn test_point_zero_addition_folds_to_the_other_operand() { + let x = Uncertain::normal(5.0, 1.0).unwrap(); + let expr = x.clone() + Uncertain::point(0.0); + + let optimized = GraphOptimizer::new().optimize(expr.node.clone()); + + // x + 0 collapses back down to just x's own node -- same shape, same size. + assert_eq!(optimized.node_count(), x.node.node_count()); + assert!(matches!(optimized, ComputationNode::Leaf { .. })); + } + + #[test] + fn test_point_times_point_folds_to_a_single_constant() { + let expr: Uncertain = Uncertain::point(2.0) * Uncertain::point(3.0); + + let optimized = GraphOptimizer::new().optimize(expr.node.clone()); + + match &optimized { + ComputationNode::Leaf { + constant_value: Some(value), + .. + } => assert!((value - 6.0).abs() < f64::EPSILON), + _ => panic!("expected a folded constant leaf"), + } + } + + #[test] + fn test_tiny_variance_normal_is_not_folded() { + // A `normal(0, 1e-12)` has genuinely nonzero variance -- it must never be + // folded, no matter how close to a point mass it looks. `Uncertain::normal` + // always builds a plain (non-`constant`) leaf, so this holds structurally. + let tiny_variance = Uncertain::normal(0.0, 1e-12).unwrap(); + assert!(!GraphOptimizer::is_constant_zero(&tiny_variance.node)); + } + + #[test] + fn test_optimized_and_unoptimized_graphs_sample_identically_under_same_seed() { + use rand::SeedableRng; + use rand_chacha::ChaCha8Rng; + + // A moderately complex expression mixing genuine identity/constant + // opportunities (`+ point(0.0)`, `* point(1.0)`) with real randomness. + let x = Uncertain::normal(3.0, 1.0).unwrap(); + let y = Uncertain::normal(2.0, 0.5).unwrap(); + let expr = ((x.clone() + y.clone()) + Uncertain::point(0.0)) + * (x.clone() - y.clone()) + * Uncertain::point(1.0); + + let optimized_node = GraphOptimizer::new().optimize(expr.node.clone()); + let optimized = Uncertain::with_node(optimized_node); + + let mut rng_a = ChaCha8Rng::seed_from_u64(2024); + let mut rng_b = ChaCha8Rng::seed_from_u64(2024); + + for _ in 0..50 { + let unoptimized_sample = expr.sample_with(&mut rng_a); + let optimized_sample = optimized.sample_with(&mut rng_b); + assert!((unoptimized_sample - optimized_sample).abs() < f64::EPSILON); + } + } + #[test] fn test_graph_visualizer_print_tree() { let left = ComputationNode::leaf(|| 1.0); @@ -1932,6 +1943,7 @@ mod tests { let leaf = ComputationNode::Leaf { id: leaf_id, sample: Arc::new(rand::random::), + constant_value: None, }; let val1 = leaf.evaluate_bool(&mut context).unwrap(); @@ -2199,29 +2211,41 @@ mod tests { #[test] fn test_check_addition_identities_both_orders_and_no_match() { let x = ComputationNode::leaf(|| 5.0); - let zero = ComputationNode::leaf(|| 0.0); - let nonzero = ComputationNode::leaf(|| 3.0); - // The "x + 0" arm matches whenever the RIGHT side is a Leaf, regardless - // of the left side, so it shadows the "0 + x" arm unless the right side - // is something other than a Leaf. - let non_leaf = ComputationNode::binary_op(x.clone(), nonzero.clone(), BinaryOperation::Add); + let zero = ComputationNode::constant(0.0); + let nonzero = ComputationNode::constant(3.0); + // The "x + 0" arm matches whenever the RIGHT side is structurally constant + // zero, regardless of the left side, so it shadows the "0 + x" arm unless + // the right side is something that isn't. + let non_constant = + ComputationNode::binary_op(x.clone(), nonzero.clone(), BinaryOperation::Add); // x + 0 = x assert!(GraphOptimizer::check_addition_identities(&x, &zero).is_some()); // 0 + x = x - assert!(GraphOptimizer::check_addition_identities(&zero, &non_leaf).is_some()); + assert!(GraphOptimizer::check_addition_identities(&zero, &non_constant).is_some()); // neither side is zero: no identity applies assert!(GraphOptimizer::check_addition_identities(&x, &nonzero).is_none()); - // left is a Leaf but not zero, right is not a Leaf: the "0 + x" arm's - // guard must actually check is_constant_zero, not always match. - assert!(GraphOptimizer::check_addition_identities(&nonzero, &non_leaf).is_none()); + // left is constant but not zero, right isn't constant at all: the "0 + x" + // arm's guard must actually check is_constant_zero, not always match. + assert!(GraphOptimizer::check_addition_identities(&nonzero, &non_constant).is_none()); + } + + #[test] + fn test_check_addition_identities_never_fires_on_sampled_zero() { + // Soundness: a leaf that always happens to sample 0.0 is not structurally + // constant (it wasn't built via `ComputationNode::constant`), so it must + // never be folded away -- constancy is never decided by sampling and + // comparing, since a low-entropy distribution could look constant by chance. + let x = ComputationNode::leaf(|| 5.0); + let looks_like_zero = ComputationNode::leaf(|| 0.0); + assert!(GraphOptimizer::check_addition_identities(&x, &looks_like_zero).is_none()); } #[test] fn test_check_subtraction_identities_match_and_no_match() { let x = ComputationNode::leaf(|| 5.0); - let zero = ComputationNode::leaf(|| 0.0); - let nonzero = ComputationNode::leaf(|| 3.0); + let zero = ComputationNode::constant(0.0); + let nonzero = ComputationNode::constant(3.0); // x - 0 = x assert!(GraphOptimizer::check_subtraction_identities(&x, &zero).is_some()); @@ -2231,35 +2255,36 @@ mod tests { #[test] fn test_check_multiplication_identities_all_orders_and_no_match() { let x = ComputationNode::leaf(|| 5.0); - let zero = ComputationNode::leaf(|| 0.0); - let one = ComputationNode::leaf(|| 1.0); - let nonzero = ComputationNode::leaf(|| 3.0); - // Same arm-shadowing consideration as addition: the "x op leaf" arms - // match whenever the right side is a Leaf, so the "leaf op x" arms - // need a non-Leaf right side to be reachable. - let non_leaf = ComputationNode::binary_op(x.clone(), nonzero.clone(), BinaryOperation::Add); + let zero = ComputationNode::constant(0.0); + let one = ComputationNode::constant(1.0); + let nonzero = ComputationNode::constant(3.0); + // Same shadowing consideration as addition: the "x op constant" arms match + // whenever the right side is structurally constant, so the "constant op x" + // arms need a non-constant right side to be reachable. + let non_constant = + ComputationNode::binary_op(x.clone(), nonzero.clone(), BinaryOperation::Add); // x * 0 = 0 assert!(GraphOptimizer::check_multiplication_identities(&x, &zero).is_some()); // 0 * x = 0 - assert!(GraphOptimizer::check_multiplication_identities(&zero, &non_leaf).is_some()); + assert!(GraphOptimizer::check_multiplication_identities(&zero, &non_constant).is_some()); // x * 1 = x assert!(GraphOptimizer::check_multiplication_identities(&x, &one).is_some()); // 1 * x = x - assert!(GraphOptimizer::check_multiplication_identities(&one, &non_leaf).is_some()); + assert!(GraphOptimizer::check_multiplication_identities(&one, &non_constant).is_some()); // no identity assert!(GraphOptimizer::check_multiplication_identities(&x, &nonzero).is_none()); - // left is a Leaf but neither zero nor one, right is not a Leaf: both - // the "0 * x" and "1 * x" arms' guards must actually check their + // left is constant but neither zero nor one, right isn't constant at all: + // both the "0 * x" and "1 * x" arms' guards must actually check their // respective is_constant_zero/is_constant_one, not always match. - assert!(GraphOptimizer::check_multiplication_identities(&nonzero, &non_leaf).is_none()); + assert!(GraphOptimizer::check_multiplication_identities(&nonzero, &non_constant).is_none()); } #[test] fn test_check_division_identities_match_and_no_match() { let x = ComputationNode::leaf(|| 5.0); - let one = ComputationNode::leaf(|| 1.0); - let nonzero = ComputationNode::leaf(|| 3.0); + let one = ComputationNode::constant(1.0); + let nonzero = ComputationNode::constant(3.0); // x / 1 = x assert!(GraphOptimizer::check_division_identities(&x, &one).is_some()); @@ -2268,37 +2293,47 @@ mod tests { #[test] fn test_is_constant_zero_and_one() { - let zero_fn: Arc f64 + Send + Sync> = Arc::new(|| 0.0); - let one_fn: Arc f64 + Send + Sync> = Arc::new(|| 1.0); - - assert!(GraphOptimizer::is_constant_zero(&zero_fn)); - assert!(!GraphOptimizer::is_constant_zero(&one_fn)); - assert!(GraphOptimizer::is_constant_one(&one_fn)); - assert!(!GraphOptimizer::is_constant_one(&zero_fn)); + let zero = ComputationNode::constant(0.0); + let one = ComputationNode::constant(1.0); + let nonzero = ComputationNode::constant(3.0); + + assert!(GraphOptimizer::is_constant_zero(&zero)); + assert!(!GraphOptimizer::is_constant_zero(&one)); + assert!(GraphOptimizer::is_constant_one(&one)); + assert!(!GraphOptimizer::is_constant_one(&zero)); + assert!(!GraphOptimizer::is_constant_zero(&nonzero)); + assert!(!GraphOptimizer::is_constant_one(&nonzero)); + + // Soundness: never decided by sampling -- a leaf that always samples 0.0/1.0 + // but wasn't built via `ComputationNode::constant` is not treated as constant. + assert!(!GraphOptimizer::is_constant_zero(&ComputationNode::leaf( + || 0.0 + ))); + assert!(!GraphOptimizer::is_constant_one(&ComputationNode::leaf( + || 1.0 + ))); } #[test] - fn test_is_constant_generic() { - let const_fn: Arc f64 + Send + Sync> = Arc::new(|| 7.0); - assert!(GraphOptimizer::is_constant(&const_fn)); - - let counter = std::sync::Arc::new(std::sync::atomic::AtomicI64::new(0)); - let counter2 = counter.clone(); - let varying_fn: Arc f64 + Send + Sync> = - Arc::new(move || counter2.fetch_add(1, std::sync::atomic::Ordering::Relaxed) as f64); - assert!(!GraphOptimizer::is_constant(&varying_fn)); + fn test_constant_folding_never_fires_on_sampled_constant() { + // A leaf that always returns the same value by construction (not a genuine + // distribution) but wasn't built via `ComputationNode::constant` must not be + // folded -- constant_folding_binary_op only checks the structural flag. + let const_looking = ComputationNode::leaf(|| 7.0); + let other = ComputationNode::leaf(|| 7.0); + let sum = ComputationNode::binary_op(const_looking, other, BinaryOperation::Add); + let folded = GraphOptimizer::constant_folding(sum); + assert!(matches!(folded, ComputationNode::BinaryOp { .. })); } #[test] - fn test_is_constant_bool() { - let const_fn: Arc bool + Send + Sync> = Arc::new(|| true); - assert!(GraphOptimizer::is_constant_bool(&const_fn)); - - let toggle = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); - let toggle2 = toggle.clone(); - let varying_fn: Arc bool + Send + Sync> = - Arc::new(move || toggle2.fetch_xor(true, std::sync::atomic::Ordering::Relaxed)); - assert!(!GraphOptimizer::is_constant_bool(&varying_fn)); + fn test_constant_folding_bool_never_fires_on_sampled_constant() { + let const_looking_condition = ComputationNode::leaf(|| true); + let if_true = ComputationNode::leaf(|| 10.0); + let if_false = ComputationNode::leaf(|| 20.0); + let conditional = ComputationNode::conditional(const_looking_condition, if_true, if_false); + let folded = GraphOptimizer::constant_folding(conditional); + assert!(matches!(folded, ComputationNode::Conditional { .. })); } #[test] diff --git a/src/distributions.rs b/src/distributions.rs index e0be0ab..e07e222 100644 --- a/src/distributions.rs +++ b/src/distributions.rs @@ -1,6 +1,7 @@ #![allow(clippy::cast_precision_loss, clippy::cast_possible_truncation)] use crate::Uncertain; +use crate::computation::ComputationNode; use crate::error::{Result, UncertainError}; use crate::rng::{random_f64, with_rng}; use crate::traits::Shareable; @@ -9,6 +10,7 @@ use rand_distr::{ Bernoulli, Beta, Binomial, Exp, Gamma, Geometric, LogNormal, Normal, Poisson, Uniform, }; use std::collections::HashMap; +use std::sync::Arc; fn validate_finite(name: &'static str, value: f64) -> Result<()> { if value.is_finite() { @@ -85,7 +87,18 @@ where /// ``` #[must_use] pub fn point(value: T) -> Self { - Uncertain::new(move || value.clone()) + let sampler: Arc T + Send + Sync> = { + let value = value.clone(); + Arc::new(move || value.clone()) + }; + let id = uuid::Uuid::new_v4(); + let node = ComputationNode::constant(value); + + Self { + id, + sample_fn: sampler, + node, + } } /// Creates a mixture of distributions with optional weights diff --git a/src/uncertain.rs b/src/uncertain.rs index 4a677c1..f96a8dc 100644 --- a/src/uncertain.rs +++ b/src/uncertain.rs @@ -60,6 +60,7 @@ where let node = ComputationNode::Leaf { id, sample: sampler.clone(), + constant_value: None, }; Self {