Repository: oliverschwendener/ueli
Pull request #1345 (2025-02-03 23:10:06)

Fixed bug with wrong results and low precision
This could be a potential fix for bug #1171
Always calculate with high precision and format the result to the desired precision if it's numeric.

oliverschwendener: Thanks for your contribution. I see that this fixes the reported issue, thanks for that! I completely misinterpreted mathjs' "precision" option.

The code is getting a bit out of thand though. I know the code was already ugly before but now it's getting even worse. For exmaple to me it's almost impossible to understand what's happening here:

```typescript
return isNaN(result) ? calculationResult : result.toFixed(precision).replace(/(\.0*|(?<=(\..*))0*)$/, "");
```

And with this fix only numeric results will be rounded (not sure if this was already a problem before) and results with units are still not rounded. For example this (I have set the precision to 2):

<img width="613" alt="image" src="https://github.com/user-attachments/assets/4b508214-801a-46fd-b62d-09a7bba0570e" />

Maybe there is a way to configure mathjs to round it's results?


tkohlmeier: If precision means the maximum number of decimal places, we should use BigNumber's `toDecimalPlaces()`, like this:

    const math = create(all, { precision, number: "BigNumber" });
    const result: BigNumber = math.evaluate(this.normalizeExpression({ expression, decimalSeparator, argumentSeparator }));
    return result.toDecimalPlaces(precision).toString();
MarcoSennHaag: > If precision means the maximum number of decimal places, we should use BigNumber's `toDecimalPlaces()`, like this:
> 
> ```
> const math = create(all, { precision, number: "BigNumber" });
> const result: BigNumber = math.evaluate(this.normalizeExpression({ expression, decimalSeparator, argumentSeparator }));
> return result.toDecimalPlaces(precision).toString();
> ```

Thank's for your input, but it looks like it's not that easy, as math.evaluate can return up to 8 different result types and some tend to throw exceptions (with or without units). I have something in mind to try. I'll keep you updated.
tkohlmeier: It's not always BigNumber although we pass `number: "BigNumber"` to create? 
MarcoSennHaag: > It's not always BigNumber although we pass `number: "BigNumber"` to create?

`number: "BigNumber"` is the format used when strings are parsed. But it has nothing to do with the result of the calculation which may or may not contain units, can be of type Complex number or Decimal. Arrays are also supported.
MarcoSennHaag: I just pushed a new version which uses a Regex to parse the number out of the result, round it and joins together everything.
Please review @oliverschwendener and/or @tkohlmeier
oliverschwendener: @MarcoSennHaag I just pushed another test use case, which I think is not covered by your fix. But in my opinion the current approach is going completely in the wrong direction. Instead of navigating our way around math.js, is there no way to configure it to return a rounded result?

@tkohlmeier Yes, the user setting "Precision" means decimal places. I completely misinterpreted that term from math.js.
MarcoSennHaag: > @MarcoSennHaag I just pushed another test use case, which I think is not covered by your fix. If the unit contains a number, for example `m^2` for square meters, it doesn't work. See all supported units of math.js here: https://mathjs.org/docs/datatypes/units.html#prefixes.
> 
> In my opinion the current approach is going completely in the wrong direction. Instead of navigating our way around math.js, is there no way to configure it to return a rounded result?
> 
> @tkohlmeier Yes, the user setting "Precision" means decimal places. I completely misinterpreted that term from math.js.

Thanks for this test case! I changed the regex to support this as well. I'm also not 100% happy with this solution as math.js should do this correctly. `math.round` allows using precision (https://mathjs.org/docs/reference/functions/round.html), but I couldn't get it to work correctly (when units are part of the result). You can give it a try or maybe use this workaround here for most common cases (better than wrong results as before). You decide!

Patch:
diff --git a/src/main/Extensions/Calculator/Calculator.ts b/src/main/Extensions/Calculator/Calculator.ts
index 6744c912..81ec0f31 100644
--- a/src/main/Extensions/Calculator/Calculator.ts
+++ b/src/main/Extensions/Calculator/Calculator.ts
@@ -72,15 +72,35 @@ export class Calculator {
             precision = 16;
         }
 
-        const math = create(all, { precision, number: "BigNumber" });
-
-        const result = String(
+        const math = create(all, { precision: 64, number: "BigNumber" });
+        const calculationResult = String(
             math.evaluate(this.normalizeExpression({ expression, decimalSeparator, argumentSeparator })),
         );
 
-        return result.replace(new RegExp(",|\\.", "g"), (match) =>
-            match === "." ? decimalSeparator : argumentSeparator,
-        );
+        let result = calculationResult;
+        try {
+            // Try to extract the number / unit using a regular expression
+            // Group 1: Number including separator
+            // Group 2: Spaces if available
+            // Group 3: Unit if available
+            const resultRegex = /([\d,.]*)(\s*)(.*)/g;
+            let matches;
+            while ((matches = resultRegex.exec(result)) !== null) {
+                // This is necessary to avoid infinite loops with zero-width matches
+                if (matches.index === resultRegex.lastIndex) {
+                    resultRegex.lastIndex++;
+                }
+
+                // Round the number with precision and put the result together again
+                result = String(math.round(math.bignumber(matches[1]), precision)) + matches[2] + matches[3];
+            }
+        } catch (error) {
+            // If any error occurs, resetting to the calculation result is better than throwing an exception
+            result = calculationResult;
+        }
+
+        result = result.replace(/[,.]/g, (match) => (match === "." ? decimalSeparator : argumentSeparator));
+        return result;
     }
 
     // eslint-disable-next-line @typescript-eslint/no-explicit-any


Test patch:
diff --git a/src/main/Extensions/Calculator/Calculator.test.ts b/src/main/Extensions/Calculator/Calculator.test.ts
index 28785c89..ea0fdc30 100644
--- a/src/main/Extensions/Calculator/Calculator.test.ts
+++ b/src/main/Extensions/Calculator/Calculator.test.ts
@@ -248,5 +248,118 @@ describe(Calculator, () => {
                 }),
             ).toEqual("5035");
         });
+
+        it("should return 5035 when evaluating the expression '5050-15' and a precision of 1", () => {
+            expect(
+                Calculator.calculate({
+                    expression: "5050-15",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 1,
+                }),
+            ).toEqual("5035");
+        });
+
+        it("should return 0.33 when evaluating the expression '(1000m in km)/3' and a precision of 2", () => {
+            expect(
+                Calculator.calculate({
+                    expression: "(1000m in km)/3",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 2,
+                }),
+            ).toEqual("0.33 km");
+        });
+
+        it("should return 33.3333 when evaluating the expression '(100m^2 in m^2)/3' and a precision of 4", () => {
+            expect(
+                Calculator.calculate({
+                    expression: "(100m^2 in m^2)/3",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 4,
+                }),
+            ).toEqual("33.3333 m^2");
+        });
+
+        it("should return 96.9 when evaluating 100-pi and a precision of 1", () => {
+            expect(
+                Calculator.calculate({
+                    expression: "100-pi",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 1,
+                }),
+            ).toEqual("96.9");
+        });
+
+        it("should return 96.8584073464 when evaluating 100-pi and a precision of 3", () => {
+            expect(
+                Calculator.calculate({
+                    expression: "100-pi",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 3,
+                }),
+            ).toEqual("96.858");
+        });
+
+        it("should return 96.8584073464 when evaluating 100-pi and a precision of 8", () => {
+            expect(
+                Calculator.calculate({
+                    expression: "100-pi",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 8,
+                }),
+            ).toEqual("96.85840735");
+        });
+
+        it("should be able to do basic evaluation (taken from https://mathjs.org/docs/expressions/parsing.html#evaluate)", () => {
+            expect(
+                Calculator.calculate({
+                    expression: "c = 2.3 + 4.5",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 4,
+                }),
+            ).toEqual("6.8");
+
+            expect(
+                Calculator.calculate({
+                    expression: "sqrt(3^2 + 4^2)",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 4,
+                }),
+            ).toEqual("5");
+
+            expect(
+                Calculator.calculate({
+                    expression: "sqrt(-4)",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 4,
+                }),
+            ).toEqual("2i");
+
+            expect(
+                Calculator.calculate({
+                    expression: "2 inch to cm",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 4,
+                }),
+            ).toEqual("5.08 cm");
+
+            expect(
+                Calculator.calculate({
+                    expression: "cos(45 deg)",
+                    argumentSeparator: ",",
+                    decimalSeparator: ".",
+                    precision: 4,
+                }),
+            ).toEqual("0.7071");
+        });
     });
 });