Floats.#23
Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 viaInt.abs(Float.toInt(r))on line 56. This means-123.45would return?(12345, -2)— the same as123.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)); };
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/FloatKey.mo`:
- Around line 30-32: The public func sort<T>(self : [var T], key : (implicit : T
-> Float)) in FloatKey.mo currently has an empty body; implement it by
decomposing the Float key into a stable integer tuple (Nat32, Int) — mirroring
the approach used in Nat32Key.mo — and then call the existing underlying sort
primitive that accepts tuple keys (i.e., create a wrapper key function that maps
each T -> (Nat32, Int) and delegate to the same sort helper used by Nat32Key).
If you cannot implement decomposition now, replace the empty body with a clear
TODO or remove the function to avoid shipping a no-op API. Ensure you reference
the function name sort and the FloatKey.mo module when making the change.
- Around line 6-11: The Decomposer currently allows maxSignificantDigits up to
15 which can produce coefficients that overflow Nat32; change the contract to
cap maxSignificantDigits at 9 by updating the assert in the Decomposer (and any
related validation) from <= 15 to <= 9 (or alternatively switch coefficient
storage/creation to a larger integer type such as Nat64 if you prefer supporting
>9 digits); update the related symbol references (Decomposer,
maxSignificantDigits, digits) so all code that constructs or casts coefficients
uses the new limit (or the larger integer type) consistently to prevent
Nat32.fromNat traps at runtime.
- Around line 15-16: The guard in public func decompose(price : Float) should
explicitly reject infinities (positive and negative) because Float.isFinite does
not exist; update the initial check in decompose to return null for infinite
values (e.g., detect infinity via a comparison such as price != price + 1 or
explicit comparisons against Float.posInfinity/negInfinity) in addition to the
existing isNaN and <= 0 checks, and also replace the incorrect use of
Float.equal with Float.equalWithin for the epsilon-based comparison elsewhere in
this file so the three-argument comparison uses the correct stdlib function
(refer to decompose and the Float.equal call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cdb5986-fc95-475c-a277-06db338ecb26
📒 Files selected for processing (2)
src/FloatKey.motest/float.test.mo
🚧 Files skipped from review as they are similar to previous changes (1)
- test/float.test.mo
| public class Decomposer(maxSignificantDigits : Nat) { | ||
| let log10 = Float.log(10); | ||
| let eps = 1e-10; | ||
|
|
||
| let digits = Float.fromInt(maxSignificantDigits); | ||
| assert maxSignificantDigits <= 15; |
There was a problem hiding this comment.
Nat32 overflow risk when maxSignificantDigits > 9.
The assertion allows up to 15 significant digits, but Nat32 has a maximum value of ~4.3×10⁹. With 10+ significant digits, the coefficient can exceed Nat32 range (e.g., coefficient 9,999,999,999 for 10 digits), causing Nat32.fromNat to trap at runtime.
Either restrict maxSignificantDigits <= 9 or use a larger integer type for the coefficient.
🐛 Proposed fix to restrict digits
let digits = Float.fromInt(maxSignificantDigits);
- assert maxSignificantDigits <= 15;
+ assert maxSignificantDigits <= 9; // Nat32 max ≈ 4.3×10^9📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class Decomposer(maxSignificantDigits : Nat) { | |
| let log10 = Float.log(10); | |
| let eps = 1e-10; | |
| let digits = Float.fromInt(maxSignificantDigits); | |
| assert maxSignificantDigits <= 15; | |
| public class Decomposer(maxSignificantDigits : Nat) { | |
| let log10 = Float.log(10); | |
| let eps = 1e-10; | |
| let digits = Float.fromInt(maxSignificantDigits); | |
| assert maxSignificantDigits <= 9; // Nat32 max ≈ 4.3×10^9 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FloatKey.mo` around lines 6 - 11, The Decomposer currently allows
maxSignificantDigits up to 15 which can produce coefficients that overflow
Nat32; change the contract to cap maxSignificantDigits at 9 by updating the
assert in the Decomposer (and any related validation) from <= 15 to <= 9 (or
alternatively switch coefficient storage/creation to a larger integer type such
as Nat64 if you prefer supporting >9 digits); update the related symbol
references (Decomposer, maxSignificantDigits, digits) so all code that
constructs or casts coefficients uses the new limit (or the larger integer type)
consistently to prevent Nat32.fromNat traps at runtime.
| public func decompose(price : Float) : ?(Nat32, Int) { | ||
| if (Float.isNaN(price) or price <= 0) return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Float.isFinite is available in Motoko core library or similar infinity check
rg -n "isFinite|isInfinite" --type-add 'motoko:*.mo' --type motokoRepository: research-ag/sort
Length of output: 42
🏁 Script executed:
cat -n src/FloatKey.mo | head -40Repository: research-ag/sort
Length of output: 1389
🌐 Web query:
Motoko Float.isFinite standard library documentation
💡 Result:
No, there is no Float.isFinite function in the Motoko standard library base/Float module. The official documentation lists functions like isNaN, abs, sqrt, equalWithin, but no isFinite or isInfinite. To check if a Float is finite, use not Float.isNaN(x) and x != +inf and x != -inf, as Motoko supports infinity and NaN representations per IEEE 754.
Citations:
- 1: https://docs.internetcomputer.org/motoko/base/Float
- 2: https://docs.internetcomputer.org/motoko/fundamentals/types/primitive-types
- 3: https://internetcomputer.org/docs/motoko/base
🏁 Script executed:
rg "Float\." src/FloatKey.mo | head -20Repository: research-ag/sort
Length of output: 457
🌐 Web query:
Motoko Float.equal vs Float.equalWithin signature documentation
💡 Result:
In the Motoko base library Float module, Float.equal has the signature: func equal(x : Float, y : Float) : Bool It returns x == y and is marked @deprecated in favor of Float.equalWithin due to not accounting for numerical errors in floating-point comparisons. Float.equalWithin has the signature: func equalWithin(x : Float, y : Float, epsilon : Float) : Bool It determines if x is approximately equal to y within the tolerance epsilon (equivalent to Float.abs(x - y) <= epsilon for epsilon >= 0). It handles special cases like signed zeros and infinities being equal, but NaN comparisons return false. This is the recommended function for floating-point equality checks.
Citations:
Add explicit check for infinity; note that Float.isFinite does not exist in Motoko.
Positive infinity passes the current guard (isNaN is false, +∞ > 0 is true) and relies on NaN propagation through arithmetic to eventually return null. This is fragile and implementation-dependent. Motoko does not provide a Float.isFinite function, but you can check for infinities explicitly or use inequality checks (e.g., price != price + 1 to detect infinity). Alternatively, reject any value outside a valid range.
Note: Line 24 also contains a separate issue—Float.equal in Motoko's stdlib accepts only 2 arguments. For the 3-argument epsilon-based comparison, use Float.equalWithin instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FloatKey.mo` around lines 15 - 16, The guard in public func
decompose(price : Float) should explicitly reject infinities (positive and
negative) because Float.isFinite does not exist; update the initial check in
decompose to return null for infinite values (e.g., detect infinity via a
comparison such as price != price + 1 or explicit comparisons against
Float.posInfinity/negInfinity) in addition to the existing isNaN and <= 0
checks, and also replace the incorrect use of Float.equal with Float.equalWithin
for the epsilon-based comparison elsewhere in this file so the three-argument
comparison uses the correct stdlib function (refer to decompose and the
Float.equal call).
| public func sort<T>(self : [var T], key : (implicit : T -> Float)) { | ||
|
|
||
| }; |
There was a problem hiding this comment.
Empty sort function body — incomplete implementation.
The function signature is defined but the body does nothing. Based on the patterns in src/Nat32Key.mo (snippets 2-3), this should decompose float keys to (Nat32, Int) tuples and delegate to the underlying sort primitives.
Either implement the sorting logic or remove/mark this as a TODO to avoid shipping dead code that silently fails to sort.
Would you like me to help draft an implementation that decomposes floats and delegates to the existing sort infrastructure?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FloatKey.mo` around lines 30 - 32, The public func sort<T>(self : [var
T], key : (implicit : T -> Float)) in FloatKey.mo currently has an empty body;
implement it by decomposing the Float key into a stable integer tuple (Nat32,
Int) — mirroring the approach used in Nat32Key.mo — and then call the existing
underlying sort primitive that accepts tuple keys (i.e., create a wrapper key
function that maps each T -> (Nat32, Int) and delegate to the same sort helper
used by Nat32Key). If you cannot implement decomposition now, replace the empty
body with a clear TODO or remove the function to avoid shipping a no-op API.
Ensure you reference the function name sort and the FloatKey.mo module when
making the change.
1 participant
Summary by CodeRabbit
New Features
Tests