From 13f352752e6d1dfc175d28e70b17c25d1996fbec Mon Sep 17 00:00:00 2001 From: Vyacheslav Matyukhin Date: Wed, 21 Sep 2022 02:20:17 +0400 Subject: [PATCH 1/5] review updates --- .../src/rescript/Utility/E/E_A.res | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/packages/squiggle-lang/src/rescript/Utility/E/E_A.res b/packages/squiggle-lang/src/rescript/Utility/E/E_A.res index 46ada15f..5bef1d32 100644 --- a/packages/squiggle-lang/src/rescript/Utility/E/E_A.res +++ b/packages/squiggle-lang/src/rescript/Utility/E/E_A.res @@ -307,9 +307,12 @@ module Floats = { continuous samples and discrete samples. The discrete samples are stored in a mutable map. Samples are thought to be discrete if they have at least `minDiscreteWight` duplicates. - If the min discreet weight is 4, that would mean that at least four elements needed from a specific + If the min discrete weight is 4, that would mean that at least four elements needed from a specific value for that to be kept as discrete. This is important because in some cases, we can expect that - some common elements will be generated by regular operations. The final continous array will be sorted. + some common elements will be generated by regular operations. The final continuous array will be sorted. + + This function is performance-critical, don't change it significantly without benchmarking + SampleSet->PointSet conversion performance. */ let splitContinuousAndDiscreteForMinWeight = ( sortedArray: array, @@ -318,34 +321,32 @@ module Floats = { let continuous: array = [] let discrete = FloatFloatMap.empty() - let flush = (cnt: int, value: float): unit => { - if cnt >= minDiscreteWeight { - FloatFloatMap.add(value, cnt->Belt.Int.toFloat, discrete) + let addData = (count: int, value: float): unit => { + if count >= minDiscreteWeight { + FloatFloatMap.add(value, count->Belt.Int.toFloat, discrete) } else { - for _ in 1 to cnt { - let _ = continuous->Js.Array2.push(value) + for _ in 1 to count { + continuous->Js.Array2.push(value)->ignore } } } - if sortedArray->Js.Array2.length != 0 { - let (finalCnt, finalValue) = sortedArray->Belt.Array.reduce( - // initial prev value doesn't matter; if it collides with the first element of the array, flush won't do anything - (0, 0.), - ((cnt, prev), element) => { - if element == prev { - (cnt + 1, prev) - } else { - // new value, process previous ones - flush(cnt, prev) - (1, element) - } - }, - ) + let (finalCount, finalValue) = sortedArray->Belt.Array.reduce( + // initial prev value doesn't matter; if it collides with the first element of the array, flush won't do anything + (0, 0.), + ((count, prev), element) => { + if element == prev { + (count + 1, prev) + } else { + // new value, process previous ones + addData(count, prev) + (1, element) + } + }, + ) - // flush final values - flush(finalCnt, finalValue) - } + // flush final values + addData(finalCount, finalValue) (continuous, discrete) } From a96f9ffa9ac8d01f6c49f5ff37785a1711370de7 Mon Sep 17 00:00:00 2001 From: Vyacheslav Matyukhin Date: Wed, 21 Sep 2022 02:25:14 +0400 Subject: [PATCH 2/5] another splitContinuousAndDiscrete test --- .../__tests__/E/splitContinuousAndDiscrete_test.res | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/squiggle-lang/__tests__/E/splitContinuousAndDiscrete_test.res b/packages/squiggle-lang/__tests__/E/splitContinuousAndDiscrete_test.res index 30f760dc..43b0019c 100644 --- a/packages/squiggle-lang/__tests__/E/splitContinuousAndDiscrete_test.res +++ b/packages/squiggle-lang/__tests__/E/splitContinuousAndDiscrete_test.res @@ -25,6 +25,12 @@ describe("Continuous and discrete splits", () => { ([1.33455, 1.432, 2.0, 2.0, 3.5, 3.5, 3.5], []), ) + makeTest( + "more general test", + prepareInputs([10., 10., 11., 11., 11., 12., 13., 13., 13., 13., 13., 14.], 3), + ([10., 10., 12., 14.], [(11., 3.), (13., 5.)]), + ) + let makeDuplicatedArray = count => { let arr = Belt.Array.range(1, count) |> E.A.fmap(float_of_int) let sorted = arr |> Belt.SortArray.stableSortBy(_, compare) From 483aaf73db132a1361da6561ee8587bb74382886 Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Wed, 21 Sep 2022 09:39:28 +0930 Subject: [PATCH 3/5] Fixes multiple plots --- packages/components/src/lib/plotParser.ts | 86 +++++++++++++---------- 1 file changed, 49 insertions(+), 37 deletions(-) diff --git a/packages/components/src/lib/plotParser.ts b/packages/components/src/lib/plotParser.ts index 3673957c..3ad247a1 100644 --- a/packages/components/src/lib/plotParser.ts +++ b/packages/components/src/lib/plotParser.ts @@ -1,5 +1,11 @@ import * as yup from "yup"; -import { SqDistribution, result, SqRecord } from "@quri/squiggle-lang"; +import { + SqValue, + SqValueTag, + SqDistribution, + result, + SqRecord, +} from "@quri/squiggle-lang"; export type LabeledDistribution = { name: string; @@ -22,47 +28,53 @@ function ok(x: a): result { const schema = yup .object() .strict() - .noUnknown() .shape({ - distributions: yup.object().shape({ - tag: yup.mixed().oneOf(["array"]), - value: yup - .array() - .of( - yup.object().shape({ - tag: yup.mixed().oneOf(["record"]), - value: yup.object({ - name: yup.object().shape({ - tag: yup.mixed().oneOf(["string"]), - value: yup.string().required(), - }), - // color: yup - // .object({ - // tag: yup.mixed().oneOf(["string"]), - // value: yup.string().required(), - // }) - // .default(undefined), - distribution: yup.object({ - tag: yup.mixed().oneOf(["distribution"]), - value: yup.mixed(), - }), - }), - }) - ) - .required(), - }), + distributions: yup + .array() + .required() + .of( + yup.object().required().shape({ + name: yup.string().required(), + distribution: yup.mixed(), + }) + ), }); +type JsonObject = + | string + | { [key: string]: JsonObject } + | JsonObject[] + | SqDistribution; + +function toJson(val: SqValue): JsonObject { + if (val.tag === SqValueTag.String) { + return val.value; + } else if (val.tag === SqValueTag.Record) { + return toJsonRecord(val.value); + } else if (val.tag === SqValueTag.Array) { + return val.value.getValues().map(toJson); + } else if (val.tag === SqValueTag.Distribution) { + return val.value; + } else { + throw new Error("Could not parse object of type " + val.tag); + } +} + +function toJsonRecord(val: SqRecord): JsonObject { + let recordObject: JsonObject = {}; + val.entries().forEach(([key, value]) => (recordObject[key] = toJson(value))); + return recordObject; +} + export function parsePlot(record: SqRecord): result { try { - const plotRecord = schema.validateSync(record); - return ok({ - distributions: plotRecord.distributions.value.map((x) => ({ - name: x.value.name.value, - // color: x.value.color?.value, // not supported yet - distribution: x.value.distribution.value, - })), - }); + const plotRecord = schema.validateSync(toJsonRecord(record)); + if (plotRecord.distributions) { + return ok({ distributions: plotRecord.distributions.map((x) => x) }); + } else { + // I have no idea why yup's typings thinks this is possible + return error("no distributions field. Should never get here"); + } } catch (e) { const message = e instanceof Error ? e.message : "Unknown error"; return error(message); From 59e937ad7c4748bf54fff59d27bf59328b99f80a Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Wed, 21 Sep 2022 09:42:21 +0930 Subject: [PATCH 4/5] Re-add noUnknown to yup schema for plots --- packages/components/src/lib/plotParser.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/src/lib/plotParser.ts b/packages/components/src/lib/plotParser.ts index 3ad247a1..a6dd477b 100644 --- a/packages/components/src/lib/plotParser.ts +++ b/packages/components/src/lib/plotParser.ts @@ -27,6 +27,7 @@ function ok(x: a): result { const schema = yup .object() + .noUnknown() .strict() .shape({ distributions: yup From 46fd5900d2506352448fadd180ba8a142e246c9f Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Wed, 21 Sep 2022 16:14:06 +1000 Subject: [PATCH 5/5] Make distributions required --- packages/components/src/lib/plotParser.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/lib/plotParser.ts b/packages/components/src/lib/plotParser.ts index a6dd477b..64e757fc 100644 --- a/packages/components/src/lib/plotParser.ts +++ b/packages/components/src/lib/plotParser.ts @@ -36,7 +36,7 @@ const schema = yup .of( yup.object().required().shape({ name: yup.string().required(), - distribution: yup.mixed(), + distribution: yup.mixed().required(), }) ), });