phetsims / dot

A math library with a focus on mutable and immutable linear algebra for 2D and 3D applications.
MIT License
13 stars 6 forks source link

return type for solveQuadraticRootsReal and solveCubicRootsReal #117

Closed veillette closed 1 year ago

veillette commented 1 year ago

Here is the method for solveQuadraticRootsReal

 /**
   * Returns an array of the real roots of the quadratic equation $ax^2 + bx + c=0$, or null if every value is a
   * solution. If a is nonzero, there should be between 0 and 2 (inclusive) values returned.
   * @public
   *
   * @param {number} a
   * @param {number} b
   * @param {number} c
   * @returns {Array.<number>|null} - The real roots of the equation, or null if all values are roots. If the root has
   *                                  a multiplicity larger than 1, it will be repeated that many times.
   */
  solveQuadraticRootsReal( a, b, c ) {
    // Check for a degenerate case where we don't have a quadratic, or if the order of magnitude is such where the
    // linear solution would be expected
    const epsilon = 1E7;
    if ( a === 0 || Math.abs( b / a ) > epsilon || Math.abs( c / a ) > epsilon ) {
      return Utils.solveLinearRootsReal( b, c );
    }

    const discriminant = b * b - 4 * a * c;
    if ( discriminant < 0 ) {
      return [];
    }
    const sqrt = Math.sqrt( discriminant );
    // TODO: how to handle if discriminant is 0? give unique root or double it?
    // TODO: probably just use Complex for the future
    return [
      ( -b - sqrt ) / ( 2 * a ),
      ( -b + sqrt ) / ( 2 * a )
    ];
  },

If the discriminant is smaller than zero, it returns an empty array.

a = []

a instanceof Array --> true
Array.isArray(a) -->true
a==null --> false
typeof a[0] --> 'undefined'

The return type is therefore never null. A similar issue jsDoc is present with solveCubicRootsReal. But undefined is not a type so I'm not sure how to indicate an empty array. Also, I'm not sure if we should correct the implementation to match the documentation or vice versa.

Assigning to @jonathanolson.

jonathanolson commented 1 year ago

solveQuadraticRootsReal( 0, 0, 0 ) returns null, so the documentation seems to match the behavior. It calls into solveLinearRootsReal, which has a return null here: https://github.com/phetsims/dot/blob/d5651b2c7a2f57f068299c5472b47b4797662fb9/js/Utils.js#L376

veillette commented 1 year ago

Gotcha. Nothing to do here. Closing.