Floats.

[AStepanov25]

Summary by CodeRabbit

• Tests
  • Added comprehensive test suite for float decomposition with test cases covering integers, decimals, small numbers, zero values, power of 10, and precision overflow scenarios to validate behavior across various input ranges.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

A new test file has been added that introduces two implementations for decomposing floats into significant digits and exponents: an original two-branch version and an improved single-branch version. The file includes a test harness that exercises both implementations across multiple scenarios including integers, decimals, and edge cases like zero and precision overflow.

Changes

Cohort / File(s)	Change Summary
New Test Suite test/float.test.mo	Adds decompose_Original() and decompose_Improved() functions for float decomposition into (Nat32, Int) pairs, plus assertEqual() and test_suite() utilities. Tests both implementations across seven scenarios: integers, decimals, values less than 1, precision overflow, small overflow, zero, and powers of 10. Total of 117 lines.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

A rabbit hops through floating land,
Where numbers dance so light and grand,
Two ways to break them down just right,
Tests verify each sum and flight, 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Floats.' is too vague and does not clearly describe the main change; it lacks specificity about what float-related work is being added or modified.	Use a more descriptive title that specifies the main change, such as 'Add float decomposition test implementations' or 'Test float-to-exponent conversion algorithms'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@test/float.test.mo`:
- Around line 11-33: In decompose_Original, use a single precise ln(10) constant
instead of the two differing literals and add handling for non-positive inputs:
define a single constant (e.g., ln10) used in both branches (replace occurrences
of 2.302_585_092_994_045 and 2.302_585_092_994_047), and at the start of
decompose_Original guard against price <= 0 by returning null (or an appropriate
sentinel) to avoid Float.log(0) and Float.log(negative) producing -infinity/NaN;
update references inside the function to use ln10 and ensure the early-return
covers zero and negative prices before any Float.log calls.

🧹 Nitpick comments (3)

test/float.test.mo (3)

38-60: Clarify negative price handling behavior.

The comment on line 41 mentions handling negative prices "safely" with abs(), but the current behavior silently converts negative prices to positive results via Int.abs(Float.toInt(r)) on line 56. This means -123.45 would return ?(12345, -2) — the same as 123.45.

If negative prices should be rejected, explicitly return null. If they should be supported, consider how sign information should be preserved or documented.

Option A: Reject negative prices

 func decompose_Improved(price : Float) : ?(Nat32, Int) {
   if (price == 0) return ?(0, 0);
+  if (price < 0) return null;

   // Uses abs() to handle potential negative prices safely
-  let logPrice = Float.log(Float.abs(price)) / 2.302_585_092_994_045;
+  let logPrice = Float.log(price) / 2.302_585_092_994_045;

Option B: Document the behavior

-  // Uses abs() to handle potential negative prices safely
+  // Negative prices are treated as their absolute value (sign is discarded)
   let logPrice = Float.log(Float.abs(price)) / 2.302_585_092_994_045;

---

66-80: Consider adding assertion traps for CI integration.

The current implementation prints pass/fail results but doesn't actually fail the test execution on assertion failures. In a CI/CD pipeline, test failures would be silent (process exits successfully).

Add trap on failure for CI

 func assertEqual(name : Text, actual : ?(Nat32, Int), expected : ?(Nat32, Int)) {
   switch (actual, expected) {
     case (null, null) { Debug.print("✅ " # name # ": PASS (Both null)") };
     case (?(a), ?(b)) {
       if (a.0 == b.0 and a.1 == b.1) {
         Debug.print("✅ " # name # ": PASS (" # debug_show (a) # ")");
       } else {
-        Debug.print("❌ " # name # ": FAIL. Got " # debug_show (a) # ", Expected " # debug_show (b));
+        Debug.trap("❌ " # name # ": FAIL. Got " # debug_show (a) # ", Expected " # debug_show (b));
       };
     };
     case (_, _) {
-      Debug.print("❌ " # name # ": FAIL. Mismatch null vs value.");
+      Debug.trap("❌ " # name # ": FAIL. Mismatch null vs value.");
     };
   };
 };

---

82-111: Consider adding test cases for negative prices.

Given the implementations handle (or don't handle) negative prices differently, adding explicit test cases would clarify the expected behavior and catch regressions.

Add negative price test cases

   // 7. Power of 10
   assertEqual("Power of 10", funcToTest(100.0), ?(10000, -2));
+
+  // 8. Negative price - define expected behavior
+  // Option A: If negatives should be rejected:
+  assertEqual("Negative Price", funcToTest(-123.45), null);
+  // Option B: If negatives should work like positives:
+  // assertEqual("Negative Price", funcToTest(-123.45), ?(12345, -2));
 };

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Inconsistent ln(10) constants and missing edge case handling.

Two issues identified:

1. Inconsistent constants: Line 13 uses 2.302_585_092_994_045 while line 23 uses 2.302_585_092_994_047. This appears to be a typo and could cause subtle precision differences between branches.

2. Missing zero/negative handling: When price == 0, Float.log(0) returns -infinity, causing undefined behavior. For negative prices, Float.log(negative) returns NaN. The improved version handles zero but this original version does not.

Proposed fix

 func decompose_Original(price : Float) : ?(Nat32, Int) {
+  if (price == 0) return ?(0, 0);
+  if (price < 0) return null; // or handle negative prices explicitly
+
   if (price >= 1) {
     let e1 = Float.log(price) / 2.302_585_092_994_045;
     let e = Float.trunc(e1);
     let exp = e + 1 - Float.fromInt(maxSignificantDigits);
     let m = 10 ** exp;
     let n = price / m;
     let r = Float.nearest(n);
     if (Float.equalWithin(n, r, 1e-10)) {
       ?(Nat32.fromNat(Int.abs(Float.toInt(r))), Float.toInt(exp));
     } else { null };
   } else {
-    let e1 = Float.log(price) / 2.302_585_092_994_047;
+    let e1 = Float.log(price) / 2.302_585_092_994_045;
     let e = Float.trunc(e1);

🤖 Prompt for AI Agents

In `@test/float.test.mo` around lines 11 - 33, In decompose_Original, use a single
precise ln(10) constant instead of the two differing literals and add handling
for non-positive inputs: define a single constant (e.g., ln10) used in both
branches (replace occurrences of 2.302_585_092_994_045 and
2.302_585_092_994_047), and at the start of decompose_Original guard against
price <= 0 by returning null (or an appropriate sentinel) to avoid Float.log(0)
and Float.log(negative) producing -infinity/NaN; update references inside the
function to use ln10 and ensure the early-return covers zero and negative prices
before any Float.log calls.