From 9090c442846e65c4647c0079da44e56c80bff9e7 Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Sat, 23 Apr 2022 15:58:42 -0400 Subject: [PATCH 1/4] Fix subtraction not commuting under pointsets --- .../PointSetDist/AlgebraicShapeCombination.res | 4 +++- .../Distributions/PointSetDist/Continuous.res | 2 ++ .../rescript/Distributions/PointSetDist/Mixed.res | 14 ++++++++++++-- .../Distributions/PointSetDist/PointSetDist.res | 7 ++++--- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res index fd7871cd..d68a9623 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res @@ -191,12 +191,14 @@ let combineShapesContinuousDiscrete = ( op: Operation.convolutionOperation, continuousShape: PointSetTypes.xyShape, discreteShape: PointSetTypes.xyShape, + flip: bool, ): PointSetTypes.xyShape => { let t1n = continuousShape |> XYShape.T.length let t2n = discreteShape |> XYShape.T.length // each x pair is added/subtracted - let fn = Operation.Convolution.toFn(op) + let opFunc = Operation.Convolution.toFn(op) + let fn = flip ? (a, b) => opFunc(b, a) : opFunc let outXYShapes: array> = Belt.Array.makeUninitializedUnsafe(t2n) diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res index a8542bae..de5d65ed 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res @@ -278,6 +278,7 @@ let combineAlgebraicallyWithDiscrete = ( op: Operation.convolutionOperation, t1: t, t2: PointSetTypes.discreteShape, + flip: bool, ) => { let t1s = t1 |> getShape let t2s = t2.xyShape // TODO would like to use Discrete.getShape here, but current file structure doesn't allow for that @@ -294,6 +295,7 @@ let combineAlgebraicallyWithDiscrete = ( op, continuousAsLinear |> getShape, t2s, + flip, ) let combinedIntegralSum = switch op { diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res index 9961b51d..da604262 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res @@ -277,8 +277,18 @@ let combineAlgebraically = (op: Operation.convolutionOperation, t1: t, t2: t): t // continuous (*) continuous => continuous, but also // discrete (*) continuous => continuous (and vice versa). We have to take care of all combos and then combine them: let ccConvResult = Continuous.combineAlgebraically(op, t1.continuous, t2.continuous) - let dcConvResult = Continuous.combineAlgebraicallyWithDiscrete(op, t2.continuous, t1.discrete) - let cdConvResult = Continuous.combineAlgebraicallyWithDiscrete(op, t1.continuous, t2.discrete) + let dcConvResult = Continuous.combineAlgebraicallyWithDiscrete( + op, + t2.continuous, + t1.discrete, + true, + ) + let cdConvResult = Continuous.combineAlgebraicallyWithDiscrete( + op, + t1.continuous, + t2.discrete, + false, + ) let continuousConvResult = Continuous.sum([ccConvResult, dcConvResult, cdConvResult]) // ... finally, discrete (*) discrete => discrete, obviously: diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res index 6dacc40f..641532d6 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res @@ -46,9 +46,10 @@ let combineAlgebraically = (op: Operation.convolutionOperation, t1: t, t2: t): t switch (t1, t2) { | (Continuous(m1), Continuous(m2)) => Continuous.combineAlgebraically(op, m1, m2) |> Continuous.T.toPointSetDist - | (Continuous(m1), Discrete(m2)) - | (Discrete(m2), Continuous(m1)) => - Continuous.combineAlgebraicallyWithDiscrete(op, m1, m2) |> Continuous.T.toPointSetDist + | (Discrete(m1), Continuous(m2)) => + Continuous.combineAlgebraicallyWithDiscrete(op, m2, m1, true) |> Continuous.T.toPointSetDist + | (Continuous(m1), Discrete(m2)) => + Continuous.combineAlgebraicallyWithDiscrete(op, m1, m2, false) |> Continuous.T.toPointSetDist | (Discrete(m1), Discrete(m2)) => Discrete.combineAlgebraically(op, m1, m2) |> Discrete.T.toPointSetDist | (m1, m2) => Mixed.combineAlgebraically(op, toMixed(m1), toMixed(m2)) |> Mixed.T.toPointSetDist From aa192883e2063e8a862784547644049a7b9d6137 Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Mon, 25 Apr 2022 15:18:18 -0400 Subject: [PATCH 2/4] Fix missing paramater in test Value: [0.001 to 0.01] --- .../__tests__/Distributions/AlgebraicShapeCombination_test.res | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res b/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res index 7b84ee99..c1e203ad 100644 --- a/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res +++ b/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res @@ -9,6 +9,7 @@ describe("Combining Continuous and Discrete Distributions", () => { #Multiply, {xs: [0., 1.], ys: [1., 1.]}, {xs: [-1.], ys: [1.]}, + false ), ), // Multiply distribution by -1 true, From 62653d55b4dc69f6c5bc4c3dc7e59a14fb69e40e Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Mon, 25 Apr 2022 15:46:56 -0400 Subject: [PATCH 3/4] Fix pointwise not commuting for subtraction --- .../ReducerInterface/ReducerInterface_Distribution_test.res | 1 + .../PointSetDist/AlgebraicShapeCombination.res | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res b/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res index 605797b9..a4569a88 100644 --- a/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res +++ b/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res @@ -54,6 +54,7 @@ describe("eval on distribution functions", () => { describe("subtract", () => { testEval("10 - normal(5, 1)", "Ok(Normal(5,1))") testEval("normal(5, 1) - 10", "Ok(Normal(-5,1))") + testEval("mean(1 - toPointSet(normal(5, 2)))", "Ok(-4.002309896304692)") }) describe("multiply", () => { testEval("normal(10, 2) * 2", "Ok(Normal(20,4))") diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res index 673f53f7..cf563d91 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res @@ -209,9 +209,13 @@ let combineShapesContinuousDiscrete = ( // creates a new continuous shape for each one of the discrete points, and collects them in outXYShapes. let dxyShape: array<(float, float)> = Belt.Array.makeUninitializedUnsafe(t1n) for i in 0 to t1n - 1 { + // When this operation is flipped (like 1 - normal(5, 2)) then the + // x axis coordinates would all come out the wrong order. So we need + // to fill them out in the opposite direction + let index = flip ? t1n - 1 - i : i Belt.Array.set( dxyShape, - i, + index, ( fn(continuousShape.xs[i], discreteShape.xs[j]), continuousShape.ys[i] *. discreteShape.ys[j], From 6bfecf22041673a1cb7afa9ea755b20db54983d8 Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Mon, 25 Apr 2022 15:55:25 -0400 Subject: [PATCH 4/4] Rename flip argument to be more descriptive --- .../AlgebraicShapeCombination_test.res | 2 +- .../PointSetDist/AlgebraicShapeCombination.res | 8 +++++--- .../Distributions/PointSetDist/Continuous.res | 4 ++-- .../rescript/Distributions/PointSetDist/Mixed.res | 4 ++-- .../Distributions/PointSetDist/PointSetDist.res | 14 ++++++++++++-- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res b/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res index c1e203ad..702b67a4 100644 --- a/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res +++ b/packages/squiggle-lang/__tests__/Distributions/AlgebraicShapeCombination_test.res @@ -9,7 +9,7 @@ describe("Combining Continuous and Discrete Distributions", () => { #Multiply, {xs: [0., 1.], ys: [1., 1.]}, {xs: [-1.], ys: [1.]}, - false + ~discretePosition=Second, ), ), // Multiply distribution by -1 true, diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res index cf563d91..63600e43 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/AlgebraicShapeCombination.res @@ -187,18 +187,20 @@ let toDiscretePointMassesFromDiscrete = (s: PointSetTypes.xyShape): pointMassesW {n: n, masses: masses, means: means, variances: variances} } +type argumentPosition = First | Second + let combineShapesContinuousDiscrete = ( op: Operation.convolutionOperation, continuousShape: PointSetTypes.xyShape, discreteShape: PointSetTypes.xyShape, - flip: bool, + ~discretePosition: argumentPosition, ): PointSetTypes.xyShape => { let t1n = continuousShape |> XYShape.T.length let t2n = discreteShape |> XYShape.T.length // each x pair is added/subtracted let opFunc = Operation.Convolution.toFn(op) - let fn = flip ? (a, b) => opFunc(b, a) : opFunc + let fn = discretePosition == First ? (a, b) => opFunc(b, a) : opFunc let outXYShapes: array> = Belt.Array.makeUninitializedUnsafe(t2n) @@ -212,7 +214,7 @@ let combineShapesContinuousDiscrete = ( // When this operation is flipped (like 1 - normal(5, 2)) then the // x axis coordinates would all come out the wrong order. So we need // to fill them out in the opposite direction - let index = flip ? t1n - 1 - i : i + let index = discretePosition == First ? t1n - 1 - i : i Belt.Array.set( dxyShape, index, diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res index de5d65ed..5e44f900 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Continuous.res @@ -278,7 +278,7 @@ let combineAlgebraicallyWithDiscrete = ( op: Operation.convolutionOperation, t1: t, t2: PointSetTypes.discreteShape, - flip: bool, + ~discretePosition: AlgebraicShapeCombination.argumentPosition, ) => { let t1s = t1 |> getShape let t2s = t2.xyShape // TODO would like to use Discrete.getShape here, but current file structure doesn't allow for that @@ -295,7 +295,7 @@ let combineAlgebraicallyWithDiscrete = ( op, continuousAsLinear |> getShape, t2s, - flip, + ~discretePosition, ) let combinedIntegralSum = switch op { diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res index da604262..4ce2bdd6 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/Mixed.res @@ -281,13 +281,13 @@ let combineAlgebraically = (op: Operation.convolutionOperation, t1: t, t2: t): t op, t2.continuous, t1.discrete, - true, + ~discretePosition=First, ) let cdConvResult = Continuous.combineAlgebraicallyWithDiscrete( op, t1.continuous, t2.discrete, - false, + ~discretePosition=Second, ) let continuousConvResult = Continuous.sum([ccConvResult, dcConvResult, cdConvResult]) diff --git a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res index 641532d6..12aa5477 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res +++ b/packages/squiggle-lang/src/rescript/Distributions/PointSetDist/PointSetDist.res @@ -47,9 +47,19 @@ let combineAlgebraically = (op: Operation.convolutionOperation, t1: t, t2: t): t | (Continuous(m1), Continuous(m2)) => Continuous.combineAlgebraically(op, m1, m2) |> Continuous.T.toPointSetDist | (Discrete(m1), Continuous(m2)) => - Continuous.combineAlgebraicallyWithDiscrete(op, m2, m1, true) |> Continuous.T.toPointSetDist + Continuous.combineAlgebraicallyWithDiscrete( + op, + m2, + m1, + ~discretePosition=First, + ) |> Continuous.T.toPointSetDist | (Continuous(m1), Discrete(m2)) => - Continuous.combineAlgebraicallyWithDiscrete(op, m1, m2, false) |> Continuous.T.toPointSetDist + Continuous.combineAlgebraicallyWithDiscrete( + op, + m1, + m2, + ~discretePosition=Second, + ) |> Continuous.T.toPointSetDist | (Discrete(m1), Discrete(m2)) => Discrete.combineAlgebraically(op, m1, m2) |> Discrete.T.toPointSetDist | (m1, m2) => Mixed.combineAlgebraically(op, toMixed(m1), toMixed(m2)) |> Mixed.T.toPointSetDist