From 1102ceb4ec5441ade3c11b02ef94cfb977d6cf87 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Sat, 23 Apr 2022 17:51:41 -0400 Subject: [PATCH] Show correct errors early on when log(distribution) has bad arguments --- .../ReducerInterface_Distribution_test.res | 6 +- .../__tests__/TS/Symbolic_test.ts | 2 +- .../Distributions/GenericDist/GenericDist.res | 56 +++++++++++++++++-- .../rescript/Reducer/Reducer_ErrorValue.res | 2 +- 4 files changed, 57 insertions(+), 9 deletions(-) diff --git a/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res b/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res index d4199f89..7b1e9653 100644 --- a/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res +++ b/packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res @@ -22,7 +22,7 @@ describe("eval on distribution functions", () => { testEval("mean(-normal(5,2))", "Ok(-5)") }) describe("to", () => { - testEval("5 to 2", "Error(Math Error: Low value must be less than high value.)") + testEval("5 to 2", "Error(Distribution Math Error: Low value must be less than high value.)") testEval("to(2,5)", "Ok(Lognormal(1.1512925464970227,0.27853260523016377))") testEval("to(-2,2)", "Ok(Normal(0,1.2159136638235384))") }) @@ -88,10 +88,10 @@ describe("eval on distribution functions", () => { describe("log", () => { testEval("log(2, uniform(5,8))", "Ok(Sample Set Distribution)") - testEval("log(normal(5,2), 3)", "Error(Math Error: Operation returned complex result)") + testEval("log(normal(5,2), 3)", "Error(Distribution Math Error: Argument Error First input of logarithm must be fully greater than 0)") testEval( "log(normal(5,2), normal(10,1))", - "Error(Math Error: Operation returned complex result)", + "Error(Distribution Math Error: Argument Error First input of logarithm must be fully greater than 0)", ) testEval("log(uniform(5,8))", "Ok(Sample Set Distribution)") testEval("log10(uniform(5,8))", "Ok(Sample Set Distribution)") diff --git a/packages/squiggle-lang/__tests__/TS/Symbolic_test.ts b/packages/squiggle-lang/__tests__/TS/Symbolic_test.ts index 1bcb1e2e..b949a513 100644 --- a/packages/squiggle-lang/__tests__/TS/Symbolic_test.ts +++ b/packages/squiggle-lang/__tests__/TS/Symbolic_test.ts @@ -12,7 +12,7 @@ describe("Symbolic mean", () => { expect(squiggleResult.value).toBeCloseTo((x + y + z) / 3); } catch (err) { expect((err as Error).message).toEqual( - "Expected squiggle expression to evaluate but got error: Math Error: Triangular values must be increasing order." + "Expected squiggle expression to evaluate but got error: Distribution Math Error: Triangular values must be increasing order." ); } } diff --git a/packages/squiggle-lang/src/rescript/Distributions/GenericDist/GenericDist.res b/packages/squiggle-lang/src/rescript/Distributions/GenericDist/GenericDist.res index 47fb2c01..cdb0b7ed 100644 --- a/packages/squiggle-lang/src/rescript/Distributions/GenericDist/GenericDist.res +++ b/packages/squiggle-lang/src/rescript/Distributions/GenericDist/GenericDist.res @@ -186,6 +186,48 @@ module AlgebraicCombination = { ->E.R2.fmap(r => DistributionTypes.SampleSet(r)) } + /* + It would be good to also do a check to make sure that probability mass for the second + operand, at value 1.0, is 0 (or approximately 0). However, we'd ideally want to check + that both the probability mass and the probability density are greater than zero. + Right now we don't yet have a way of getting probability mass, so I'll leave this for later. + */ + let getLogarithmInputError = (t1: t, t2: t, ~toPointSetFn: toPointSetFn): option => { + let firstOperandIsGreaterThanZero = + toFloatOperation(t1, ~toPointSetFn, ~distToFloatOperation=#Cdf(1e-10)) |> E.R.fmap(r => + r > 0. + ) + let secondOperandIsGreaterThanZero = + toFloatOperation(t2, ~toPointSetFn, ~distToFloatOperation=#Cdf(1e-10)) |> E.R.fmap(r => + r > 0. + ) + let items = E.A.R.firstErrorOrOpen([ + firstOperandIsGreaterThanZero, + secondOperandIsGreaterThanZero, + ]) + switch items { + | Error(r) => Some(r) + | Ok([true, _]) => Some(ArgumentError("First input of logarithm must be fully greater than 0")) + | Ok([false, true]) => + Some(ArgumentError("Second input of logarithm must be fully greater than 0")) + | Ok([false, false]) => None + | Ok(_) => Some(Unreachable) + } + } + + let getInvalidOperationError = ( + t1: t, + t2: t, + ~toPointSetFn: toPointSetFn, + ~arithmeticOperation, + ): option => { + if arithmeticOperation == #Logarithm { + getLogarithmInputError(t1, t2, ~toPointSetFn) + } else { + None + } + } + //I'm (Ozzie) really just guessing here, very little idea what's best let expectedConvolutionCost: t => int = x => switch x { @@ -226,10 +268,16 @@ module AlgebraicCombination = { | Some(Ok(symbolicDist)) => Ok(Symbolic(symbolicDist)) | Some(Error(e)) => Error(OperationError(e)) | None => - switch chooseConvolutionOrMonteCarlo(arithmeticOperation, t1, t2) { - | MonteCarlo => runMonteCarlo(toSampleSetFn, arithmeticOperation, t1, t2) - | Convolution(convOp) => - runConvolution(toPointSetFn, convOp, t1, t2)->E.R2.fmap(r => DistributionTypes.PointSet(r)) + switch getInvalidOperationError(t1, t2, ~toPointSetFn, ~arithmeticOperation) { + | Some(e) => Error(e) + | None => + switch chooseConvolutionOrMonteCarlo(arithmeticOperation, t1, t2) { + | MonteCarlo => runMonteCarlo(toSampleSetFn, arithmeticOperation, t1, t2) + | Convolution(convOp) => + runConvolution(toPointSetFn, convOp, t1, t2)->E.R2.fmap(r => DistributionTypes.PointSet( + r, + )) + } } } } diff --git a/packages/squiggle-lang/src/rescript/Reducer/Reducer_ErrorValue.res b/packages/squiggle-lang/src/rescript/Reducer/Reducer_ErrorValue.res index 48fae58c..96b73fd2 100644 --- a/packages/squiggle-lang/src/rescript/Reducer/Reducer_ErrorValue.res +++ b/packages/squiggle-lang/src/rescript/Reducer/Reducer_ErrorValue.res @@ -21,7 +21,7 @@ let errorToString = err => | REAssignmentExpected => "Assignment expected" | REExpressionExpected => "Expression expected" | REFunctionExpected(msg) => `Function expected: ${msg}` - | REDistributionError(err) => `Math Error: ${DistributionTypes.Error.toString(err)}` + | REDistributionError(err) => `Distribution Math Error: ${DistributionTypes.Error.toString(err)}` | REJavaScriptExn(omsg, oname) => { let answer = "JS Exception:" let answer = switch oname {