Merge pull request #359 from quantified-uncertainty/log-inputs-errors

Show correct errors early on when log(distribution) has bad arguments

packages/squiggle-lang/__tests__/ReducerInterface/ReducerInterface_Distribution_test.res

@@ -23,7 +23,7 @@ describe("eval on distribution functions", () => {
     testEval("-normal(5,2)", "Ok(Normal(-5,2))")
   })
   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))")
   })
@@ -90,10 +90,13 @@ 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: Logarithm of input error: First input must completely greater than 0)",
+    )
     testEval(
       "log(normal(5,2), normal(10,1))",
-      "Error(Math Error: Operation returned complex result)",
+      "Error(Distribution Math Error: Logarithm of input error: First input must completely greater than 0)",
     )
     testEval("log(uniform(5,8))", "Ok(Sample Set Distribution)")
     testEval("log10(uniform(5,8))", "Ok(Sample Set Distribution)")

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."
             );
           }
         }

packages/squiggle-lang/src/rescript/Distributions/DistributionTypes.res

@@ -14,6 +14,7 @@ type error =
   | OperationError(Operation.Error.t)
   | PointSetConversionError(SampleSetDist.pointsetConversionError)
   | SparklineError(PointSetTypes.sparklineError) // This type of error is for when we find a sparkline of a discrete distribution. This should probably at some point be actually implemented
+  | LogarithmOfDistributionError(string)
   | OtherError(string)
 
 @genType
@@ -29,6 +30,7 @@ module Error = {
     | Unreachable => "Unreachable"
     | DistributionVerticalShiftIsInvalid => "Distribution Vertical Shift is Invalid"
     | ArgumentError(s) => `Argument Error ${s}`
+    | LogarithmOfDistributionError(s) => `Logarithm of input error: ${s}`
     | TooFewSamples => "Too Few Samples"
     | OperationError(err) => Operation.Error.toString(err)
     | PointSetConversionError(err) => SampleSetDist.pointsetConversionErrorToString(err)

packages/squiggle-lang/src/rescript/Distributions/GenericDist/GenericDist.res

@@ -186,6 +186,49 @@ 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<error> => {
+    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(LogarithmOfDistributionError("First input must completely greater than 0"))
+    | Ok([false, true]) =>
+      Some(LogarithmOfDistributionError("Second input must completely greater than 0"))
+    | Ok([false, false]) => None
+    | Ok(_) => Some(Unreachable)
+    }
+  }
+
+  let getInvalidOperationError = (
+    t1: t,
+    t2: t,
+    ~toPointSetFn: toPointSetFn,
+    ~arithmeticOperation,
+  ): option<error> => {
+    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 +269,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,
+          ))
+        }
       }
     }
   }

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 {