feat(math) Add complex number support for the @std/math library#7122
feat(math) Add complex number support for the @std/math library#7122Definitely-Not-A-Dolphin wants to merge 17 commits into
@std/math library#7122Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7122 +/- ##
==========================================
- Coverage 94.61% 0 -94.62%
==========================================
Files 634 0 -634
Lines 51799 0 -51799
Branches 9329 0 -9329
==========================================
- Hits 49009 0 -49009
+ Misses 2216 0 -2216
+ Partials 574 0 -574 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
@std/math library@std/math library
|
Excuse me for my bad commit messages earlier, I forgot that this was a professional setting. A few topics of discussion here: StructureThe entire module contains a
DocumentationThe deno contributing guidelines state that when applicable, all exported symbols must be documented according to the following rules:
The first rule I understand, but the rules about the JSDoc documentation feel a bit unnecessary to me. Unlike all the other libraries, which are about programming / computer topics, this is math. Function names about computer topics are always at least a little vague, but math functions aren't. Sine is always sine, without any ambiguity. Also, is it really necessary to include example cases for all 24 trig functions? Not only are there a lot of them, they are all pretty abstract. I don't think that adding examples would be of any additional value. Trig function namesShould inverse trig functions be called |
|
This PR is also a draft because
|
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks for the contribution — complex number support is a genuinely useful addition. Marking this as a high-level review while it's still a draft. A few overarching points before the inline notes:
API placement. @std/math is stable, so new APIs of this scope need to be introduced under an unstable-complex export first (see the precedent in data-structures/unstable, async/unstable-*, etc.). The current diff also doesn't wire anything into math/deno.json, so the new API isn't actually exposed via the package entrypoints.
API shape. Using Complex as a namespace of ~35 static methods isn't idiomatic for std — the package prefers one named function per file (see clamp.ts, modulo.ts, round_to.ts), or, for value types, instance methods (a.add(b).mul(c)). Could you open an issue to settle the shape before we keep iterating? The choice between static-only / instance methods / per-file functions has big downstream consequences.
Scope. The author flags ~12 inverse functions as broken (branch cuts). I'd strongly suggest dropping those from the first PR and adding them in a follow-up once correct — shipping known-broken functions in an initial release sets a bad precedent.
Docs/tests. Every public API in std requires an @example Usage block with executable code that uses assertEquals / assertAlmostEquals (see math/clamp.ts:12-19). None of the new methods have examples, and the test file only covers constructor and add (with incorrect expectations — see inline). Before this is merge-ready every operation needs round-tripping tests (conj(conj(z)) === z, mul(i, i) === -1, exp(i*PI) ≈ -1, etc.).
Detailed correctness issues inline.
|
|
||
| /** | ||
| * Returns the sum of the supplied complex numbers. | ||
| * |
There was a problem hiding this comment.
Bug: the imaginary component uses num.real instead of num.imag. The existing test happens to pass because the imaginary parts in the inputs equal the real parts of the next operand. Should be:
sum = new Complex(sum.real + num.real, sum.imag + num.imag);| this.#imag = imag ?? 0; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Two problems here:
- Typo —
Number.isNaN(z.real) || Number.isNaN(z.real)checksrealtwice; the second should bez.imag. - This method is never called anywhere.
More importantly, the singleton-based NaN propagation throughout the file is broken: Complex.#complexNaN is one specific instance, but new Complex(NaN, ...) returns a new instance with NaN fields. Reference comparisons like nums.includes(this.#complexNaN) and x === this.#complexNaN in add/sub/mul/div will never match a user-constructed NaN. Replace the identity checks with Number.isNaN(z.real) || Number.isNaN(z.imag).
| this.#imag = NaN; | ||
| } else { | ||
| this.#real = real; | ||
| this.#imag = imag ?? 0; |
There was a problem hiding this comment.
See note on #NaNChecker — this singleton can't actually be detected via === against values produced by new Complex(NaN), so all the nums.includes(this.#complexNaN) / x === this.#complexNaN guards below are no-ops in practice.
| } | ||
|
|
||
| /** | ||
| * Returns the square of the absolute value of a complex number. |
There was a problem hiding this comment.
JSDoc copy-paste: this is arg, not absSquared. The @returns line still says "the square of the absolute value of the supplied number."
| } | ||
|
|
||
| get imag() { | ||
| return this.#imag; |
There was a problem hiding this comment.
Constructor only special-cases NaN, not Infinity. The tests on complex_test.ts:13-22 assume new Complex(Infinity, 1) collapses to (Infinity, Infinity), which doesn't happen. Either:
- decide that
Complexpreserves component values verbatim (recommended — matches how IEEE complex usually behaves), and fix the tests, or - add explicit Infinity normalization here and document it.
Also: imag is number | undefined, and Number.isNaN(undefined) is false, so the current branch is fine — but it's worth narrowing imag to number by defaulting earlier.
|
|
||
| return new Complex(expReal * Math.cos(z.imag), expReal * Math.sin(z.imag)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggest dropping asin/acos/atan/acot/asec/acsc and their hyperbolic counterparts from this PR entirely — you've flagged them as broken at the top of the file. Land the well-tested operations first and add inverse trig in a follow-up once branch cuts are correct.
| await t.step("constructor", () => { | ||
| assertEquals(new Complex(5, 1).imag, 1); | ||
| assertEquals(new Complex(5, 1).real, 5); | ||
|
|
There was a problem hiding this comment.
These Infinity assertions will fail as written — the constructor doesn't collapse (Infinity, 1) to (Infinity, Infinity), it stores the components as given. Either change the constructor's contract or update these expectations to new Complex(Infinity, 1) etc.
|
|
||
| assertEquals( | ||
| Complex.add(new Complex(2, 3), new Complex(2, Infinity)), | ||
| new Complex(0), |
There was a problem hiding this comment.
This expectation is mathematically wrong: Complex(2, 3) + Complex(2, Infinity) = (4, Infinity), not (0, 0). (It currently "passes" only because of the sum.imag + num.real bug in add() combined with NaN comparison quirks.)
| import { Complex } from "./complex.ts"; | ||
| import { assertEquals } from "@std/assert"; | ||
|
|
||
| Deno.test("Complex", async (t) => { |
There was a problem hiding this comment.
Test coverage needs to grow significantly before this is merge-ready — only constructor and add are exercised. At minimum every method needs a test, ideally using algebraic identities (conj(conj(z)) === z, mul(i, i) === Complex(-1), exp(Complex(0, PI)) ≈ Complex(-1), sqrt(z) * sqrt(z) === z, etc.) with assertAlmostEquals from @std/assert for floating-point comparisons.
| * @module | ||
| */ | ||
|
|
||
| export * from "./complex.ts"; |
There was a problem hiding this comment.
Two issues:
@std/mathis a stable package; new APIs of this scope must land under anunstable-complexsubpath first (e.g../unstable-complex.tsexported as"./unstable-complex").- The new file isn't added to
math/deno.json'sexportsmap, so even users who want it can't import it as@std/math/complex.
|
I finished all the functions and made all the tests pass after not having a lot of time to work on it due to school, but here I am. Here are some choices I made. Static methodsIt seemed logical for me to put all the static math methods on the class is because the of Inverse functionsAt the time of reviewing the pr was unfinished, but now I managed to make all inverse functions work, except for the cotangent, secant and cosecant, including their hyperbolic forms. I did this because I couldn't find a clear consensus as to where the branch cut was. For example, my two most used tools for this, desmos and wolframalpha, disargreed on some hyperbolic arccotangent values, which is why I omitted it plus others. |
|
Maybe doing the |
@std/math library@std/math library
No description provided.