Merge pull request #94 from concord-consortium/186820341-rounding

New rounding rule

src/diagram/components/ui/variable-chip.test.tsx

@@ -15,22 +15,22 @@ describe("VariableChip", () => {
     expect(container).toHaveTextContent(/^some name$/);
 
     applySnapshot(variable, {id: variable.id, value: 1.234});
-    expect(container).toHaveTextContent(/^1.234$/);
+    expect(container).toHaveTextContent(/^1.23$/);
 
     applySnapshot(variable, {id: variable.id, unit: "m"});
     expect(container).toHaveTextContent(/^\(m\)$/);
 
     applySnapshot(variable, {id: variable.id, name: "some name", value: 1.234});
-    expect(container).toHaveTextContent(/^some name=1.234$/);
+    expect(container).toHaveTextContent(/^some name=1.23$/);
 
     applySnapshot(variable, {id: variable.id, name: "some name", unit: "m"});
     expect(container).toHaveTextContent(/^some name\(m\)$/);
 
     applySnapshot(variable, {id: variable.id, value: 1.234, unit: "m" });
-    expect(container).toHaveTextContent(/^1.234m$/);
+    expect(container).toHaveTextContent(/^1.23m$/);
 
     applySnapshot(variable, {id: variable.id, name: "some name", value: 1.234, unit: "m" });
-    expect(container).toHaveTextContent(/^some name=1.234m$/);
+    expect(container).toHaveTextContent(/^some name=1.23m$/);
   });
   it("properly renders name only variable chips", () => {
     const variable = Variable.create();
@@ -41,7 +41,7 @@ describe("VariableChip", () => {
     expect(container).toHaveTextContent(/^some name$/);
 
     applySnapshot(variable, {id: variable.id, value: 1.234});
-    expect(container).toHaveTextContent(/^1.234$/);
+    expect(container).toHaveTextContent(/^1.23$/);
 
     applySnapshot(variable, {id: variable.id, unit: "m"});
     expect(container).toHaveTextContent(/^\(m\)$/);
@@ -53,7 +53,7 @@ describe("VariableChip", () => {
     expect(container).toHaveTextContent(/^some name$/);
 
     applySnapshot(variable, {id: variable.id, value: 1.234, unit: "m" });
-    expect(container).toHaveTextContent(/^1.234m$/);
+    expect(container).toHaveTextContent(/^1.23m$/);
 
     applySnapshot(variable, {id: variable.id, name: "some name", value: 1.234, unit: "m" });
     expect(container).toHaveTextContent(/^some name$/);

src/diagram/models/variable.test.ts

@@ -41,7 +41,7 @@ describe("Variable", () => {
     expect(variable.computedValueIncludingMessageAndError).toEqual({value: 123.5});
     expect(variable.computedUnitIncludingMessageAndError).toEqual({});
     expect(variable.computedValue).toBe(123.5);
-    expect(variable.computedValueWithSignificantDigits).toBe("123.5");
+    expect(variable.computedValueWithSignificantDigits).toBe("124");
     expect(variable.computedValueError).toBeUndefined();
     expect(variable.computedValueMessage).toBeUndefined();
     expect(variable.computedUnit).toBeUndefined();
@@ -188,7 +188,7 @@ describe("Variable", () => {
     expect(variable.computedValueIncludingMessageAndError).toEqual({value: 999.9});
     expect(variable.computedUnitIncludingMessageAndError).toEqual({unit: "mm"});
     expect(variable.computedValue).toBe(999.9);
-    expect(variable.computedValueWithSignificantDigits).toBe("999.9");
+    expect(variable.computedValueWithSignificantDigits).toBe("1,000");
     expect(variable.computedValueError).toBeUndefined();
     expect(variable.computedUnit).toBe("mm");
     expect(variable.computedUnitError).toBeUndefined();

src/diagram/models/variable.ts

@@ -6,6 +6,7 @@ import { getUsedInputs, replaceInputNames } from "../utils/mathjs-utils";
 import { createMath } from "../custom-mathjs";
 import { basicErrorMessage, ErrorMessage, getErrorMessage } from "../utils/error";
 import { Colors, legacyColors } from "../utils/theme-utils";
+import { roundForDisplay } from "../utils/math-utils";
 import { UnitsManager } from "../units-manager";
 
 export enum Operation {
@@ -353,9 +354,10 @@ export const Variable = types.model("Variable", {
     if (value === undefined) {
       return "NaN";
     }
+    const rounded = roundForDisplay(value, 3);
     // The first argument is the locale, using undefined means it should pick up the default
     // browser locale
-    return new Intl.NumberFormat(undefined, { maximumFractionDigits: 4 }).format(value);
+    return new Intl.NumberFormat(undefined, { maximumFractionDigits: 20 }).format(rounded);
   },
   get computedValueError() {
     return self.computedValueIncludingMessageAndError.error;

src/diagram/utils/math-utils.test.ts

@@ -0,0 +1,21 @@
+import { roundForDisplay } from "./math-utils";
+
+describe("roundForDisplay", () => {
+  it("should respect the number of decimal places requested", () => {
+    expect(roundForDisplay(Math.PI, 4)).toBe(3.142);
+    expect(roundForDisplay(Math.PI, 3)).toBe(3.14);
+    expect(roundForDisplay(Math.PI, 2)).toBe(3.1);
+    expect(roundForDisplay(Math.PI, 1)).toBe(3);
+  });
+  it("should handle very small numbers", () => {
+    expect(roundForDisplay(.00000039234, 3)).toBe(0.000000392);
+    expect(roundForDisplay(.00000039234, 2)).toBe(0.00000039);
+    expect(roundForDisplay(.00000039234, 1)).toBe(0.0000004);
+  });
+  it("should never remove units", () => {
+    expect(roundForDisplay(123456.1, 3)).toBe(123456);
+    expect(roundForDisplay(123456.1, 2)).toBe(123456);
+    expect(roundForDisplay(123456.1, 1)).toBe(123456);
+    expect(roundForDisplay(17.6, 1)).toBe(18);
+  });
+});

src/diagram/utils/math-utils.ts

@@ -0,0 +1,16 @@
+/**
+ * Rounds a number in an intuitive way.
+ * The digits parameter is used as a number of significant digits to maintain for small numbers.
+ * However, rounding never carries beyond the integer part of a number, even if it contains more than
+ * that number of significant digits.
+ * @param x number to round
+ * @param digits number of digits of precision to maintain in decimals
+ * @returns a rounded number
+ */
+export function roundForDisplay(x: number, digits: number): number {
+  const intRange = Math.pow(10, digits);
+  if (x<-intRange || x>intRange) {
+    return Math.round(x);
+  }
+  return +x.toPrecision(digits);
+}