Redefine min()/max() and introduce minmax() function (#10161)

* Skip NaN in {min,max}() and add minmax()

* Replace min() and max() usage with minmax()

* Fix Object.is() polyfill

bokehjs/src/lib/core/util/arrayable.ts

@@ -1,9 +1,17 @@
 import {Arrayable, ArrayableNew} from "../types"
+import {isArray} from "./types"
 
 export function is_empty(array: Arrayable): boolean {
   return array.length == 0
 }
 
+export function copy<T>(array: Arrayable<T>): Arrayable<T> {
+  if (isArray<T>(array))
+    return array.slice()
+  else
+    return new (array.constructor as any)(array)
+}
+
 export function splice<T>(array: Arrayable<T>, start: number, k?: number, ...items: T[]): Arrayable<T> {
   const len = array.length
 
@@ -122,41 +130,61 @@ export function min(array: Arrayable<number>): number {
 
   for (let i = 0, length = array.length; i < length; i++) {
     value = array[i]
-    if (value < result) {
+    if (!isNaN(value) && value < result) {
       result = value
     }
   }
 
   return result
 }
 
-export function min_by<T>(array: Arrayable<T>, key: (item: T) => number): T {
-  if (array.length == 0)
-    throw new Error("min_by() called with an empty array")
-
-  let result = array[0]
-  let resultComputed = key(result)
+export function max(array: Arrayable<number>): number {
+  let value: number
+  let result = -Infinity
 
-  for (let i = 1, length = array.length; i < length; i++) {
-    const value = array[i]
-    const computed = key(value)
-    if (computed < resultComputed) {
+  for (let i = 0, length = array.length; i < length; i++) {
+    value = array[i]
+    if (!isNaN(value) && value > result) {
       result = value
-      resultComputed = computed
     }
   }
 
   return result
 }
 
-export function max(array: Arrayable<number>): number {
+export function minmax(array: Arrayable<number>): [number, number] {
   let value: number
-  let result = -Infinity
+  let min = +Infinity
+  let max = -Infinity
 
   for (let i = 0, length = array.length; i < length; i++) {
     value = array[i]
-    if (value > result) {
+    if (!isNaN(value)) {
+      if (value < min) {
+        min = value
+      }
+      if (value > max) {
+        max = value
+      }
+    }
+  }
+
+  return [min, max]
+}
+
+export function min_by<T>(array: Arrayable<T>, key: (item: T) => number): T {
+  if (array.length == 0)
+    throw new Error("min_by() called with an empty array")
+
+  let result = array[0]
+  let resultComputed = key(result)
+
+  for (let i = 1, length = array.length; i < length; i++) {
+    const value = array[i]
+    const computed = key(value)
+    if (computed < resultComputed) {
       result = value
+      resultComputed = computed
     }
   }
 

bokehjs/src/lib/models/glyphs/image_url.ts

@@ -2,7 +2,7 @@ import {XYGlyph, XYGlyphView, XYGlyphData} from "./xy_glyph"
 import {Arrayable, Rect} from "core/types"
 import {Anchor} from "core/enums"
 import * as p from "core/properties"
-import {map, min, max} from "core/util/arrayable"
+import {map, minmax} from "core/util/arrayable"
 import {Context2d} from "core/util/canvas"
 import {SpatialIndex} from "core/util/spatial"
 import {ImageLoader} from "core/util/image"
@@ -89,10 +89,8 @@ export class ImageURLView extends XYGlyphView {
         ys[n + i] = this._y[i] + this._h[i]
     }
 
-    const x0 = min(xs)
-    const x1 = max(xs)
-    const y0 = min(ys)
-    const y1 = max(ys)
+    const [x0, x1] = minmax(xs)
+    const [y0, y1] = minmax(ys)
 
     this._bounds_rect = {x0, x1, y0, y1}
   }

bokehjs/src/lib/models/glyphs/multi_line.ts

@@ -5,19 +5,19 @@ import {Line} from "core/visuals"
 import {Arrayable, Rect} from "core/types"
 import * as hittest from "core/hittest"
 import * as p from "core/properties"
-import {min, max} from "core/util/array"
+import {minmax} from "core/util/arrayable"
 import {to_object} from "core/util/object"
 import {Context2d} from "core/util/canvas"
 import {Glyph, GlyphView, GlyphData} from "./glyph"
 import {generic_line_legend, line_interpolation} from "./utils"
 import {Selection} from "../selections/selection"
 
 export interface MultiLineData extends GlyphData {
-  _xs: Arrayable<Arrayable<number>>
-  _ys: Arrayable<Arrayable<number>>
+  _xs: Arrayable<number>[]
+  _ys: Arrayable<number>[]
 
-  sxs: Arrayable<Arrayable<number>>
-  sys: Arrayable<Arrayable<number>>
+  sxs: Arrayable<number>[]
+  sys: Arrayable<number>[]
 }
 
 export interface MultiLineView extends MultiLineData {}
@@ -29,27 +29,16 @@ export class MultiLineView extends GlyphView {
   protected _index_data(): SpatialIndex {
     const points = []
     for (let i = 0, end = this._xs.length; i < end; i++) {
-      if (this._xs[i] == null || this._xs[i].length === 0)
+      const xsi = this._xs[i]
+      if (xsi == null || xsi.length == 0) // XXX: null?, so include in types
         continue
 
-      const _xsi = this._xs[i]
-      const xs: number[] = []
-      for (let j = 0, n = _xsi.length; j < n; j++) {
-        const x = _xsi[j]
-        if (!isNaN(x))
-          xs.push(x)
-      }
-
-      const _ysi = this._ys[i]
-      const ys: number[] = []
-      for (let j = 0, n = _ysi.length; j < n; j++) {
-        const y = _ysi[j]
-        if (!isNaN(y))
-          ys.push(y)
-      }
+      const ysi = this._ys[i]
+      if (ysi == null || ysi.length == 0) // XXX: null?, so include in types
+        continue
 
-      const [x0, x1] = [min(xs), max(xs)]
-      const [y0, y1] = [min(ys), max(ys)]
+      const [x0, x1] = minmax(xsi)
+      const [y0, y1] = minmax(ysi)
 
       points.push({x0, y0, x1, y1, i})
     }

bokehjs/src/lib/models/glyphs/multi_polygons.ts

@@ -1,7 +1,7 @@
 import {SpatialIndex} from "core/util/spatial"
 import {Glyph, GlyphView, GlyphData} from "./glyph"
 import {generic_area_legend} from "./utils"
-import {min, max} from "core/util/array"
+import {minmax} from "core/util/arrayable"
 import {sum} from "core/util/arrayable"
 import {Arrayable, Rect} from "core/types"
 import {PointGeometry, RectGeometry} from "core/geometry"
@@ -40,7 +40,10 @@ export class MultiPolygonsView extends GlyphView {
         if (xs.length == 0)
           continue
 
-        points.push({x0: min(xs), y0: min(ys), x1: max(xs), y1: max(ys), i})
+        const [x0, x1] = minmax(xs)
+        const [y0, y1] = minmax(ys)
+
+        points.push({x0, y0, x1, y1, i})
       }
     }
     this.hole_index = this._index_hole_data()  // should this be set here?
@@ -60,7 +63,10 @@ export class MultiPolygonsView extends GlyphView {
             if (xs.length == 0)
               continue
 
-            points.push({x0: min(xs), y0: min(ys), x1: max(xs), y1: max(ys), i})
+            const [x0, x1] = minmax(xs)
+            const [y0, y1] = minmax(ys)
+
+            points.push({x0, y0, x1, y1, i})
           }
         }
       }

bokehjs/src/lib/models/glyphs/patches.ts

@@ -1,8 +1,8 @@
 import {SpatialIndex} from "core/util/spatial"
 import {Glyph, GlyphView, GlyphData} from "./glyph"
 import {generic_area_legend} from "./utils"
-import {min, max, copy, find_last_index} from "core/util/array"
-import {sum} from "core/util/arrayable"
+import {find_last_index} from "core/util/array"
+import {minmax, sum} from "core/util/arrayable"
 import {Arrayable, Rect} from "core/types"
 import {PointGeometry, RectGeometry} from "core/geometry"
 import {Context2d} from "core/util/canvas"
@@ -14,14 +14,14 @@ import {Selection} from "../selections/selection"
 import {unreachable} from "core/util/assert"
 
 export interface PatchesData extends GlyphData {
-  _xs: Arrayable<Arrayable<number>>
-  _ys: Arrayable<Arrayable<number>>
+  _xs: Arrayable<number>[]
+  _ys: Arrayable<number>[]
 
-  sxs: Arrayable<Arrayable<number>>
-  sys: Arrayable<Arrayable<number>>
+  sxs: Arrayable<number>[]
+  sys: Arrayable<number>[]
 
-  sxss: number[][][]
-  syss: number[][][]
+  sxss: Arrayable<number>[][]
+  syss: Arrayable<number>[][]
 }
 
 export interface PatchesView extends PatchesData {}
@@ -30,7 +30,7 @@ export class PatchesView extends GlyphView {
   model: Patches
   visuals: Patches.Visuals
 
-  private _build_discontinuous_object(nanned_qs: number[][]): number[][][] {
+  private _build_discontinuous_object(nanned_qs: Arrayable<number>[]): Arrayable<number>[][] {
     // _s is this.xs, this.ys, this.sxs, this.sys
     // an object of n 1-d arrays in either data or screen units
     //
@@ -50,7 +50,7 @@ export class PatchesView extends GlyphView {
     const ds: number[][][] = []
     for (let i = 0, end = nanned_qs.length; i < end; i++) {
       ds[i] = []
-      let qs = copy(nanned_qs[i])
+      let qs = [...nanned_qs[i]]
       while (qs.length > 0) {
         const nan_index = find_last_index(qs, (q) => isNaN(q))
 
@@ -70,8 +70,8 @@ export class PatchesView extends GlyphView {
   }
 
   protected _index_data(): SpatialIndex {
-    const xss = this._build_discontinuous_object(this._xs as any) // XXX
-    const yss = this._build_discontinuous_object(this._ys as any) // XXX
+    const xss = this._build_discontinuous_object(this._xs)
+    const yss = this._build_discontinuous_object(this._ys)
 
     const points = []
     for (let i = 0, end = this._xs.length; i < end; i++) {
@@ -82,7 +82,10 @@ export class PatchesView extends GlyphView {
         if (xs.length == 0)
           continue
 
-        points.push({x0: min(xs), y0: min(ys), x1: max(xs), y1: max(ys), i})
+        const [x0, x1] = minmax(xs)
+        const [y0, y1] = minmax(ys)
+
+        points.push({x0, y0, x1, y1, i})
       }
     }
 
@@ -123,8 +126,8 @@ export class PatchesView extends GlyphView {
   protected _render(ctx: Context2d, indices: number[], {sxs, sys}: PatchesData): void {
     // this.sxss and this.syss are used by _hit_point and sxc, syc
     // This is the earliest we can build them, and only build them once
-    this.sxss = this._build_discontinuous_object(sxs as any) // XXX
-    this.syss = this._build_discontinuous_object(sys as any) // XXX
+    this.sxss = this._build_discontinuous_object(sxs)
+    this.syss = this._build_discontinuous_object(sys)
 
     for (const i of indices) {
       const [sx, sy] = [sxs[i], sys[i]]

bokehjs/src/lib/polyfill.ts

@@ -1,4 +1,3 @@
-import "es5-ext/object/is/implement"
 import "es5-ext/object/assign/implement"
 import "es5-ext/object/entries/implement"
 import "es5-ext/number/is-integer/implement"
@@ -12,6 +11,12 @@ import "es6-weak-map/implement"
 
 import "es6-promise/auto"
 
+if (typeof Object.is === "undefined") {
+  Object.is = function(val1: any, val2: any): boolean {
+    return val1 === val2 ? val1 !== 0 || 1 / val1 === 1 / val2 : isNaN(val1) && isNaN(val2)
+  }
+}
+
 if (typeof Object.values === "undefined") {
   Object.values = function(obj: {[key: string]: unknown}) {
     return Object.keys(obj).map((key) => obj[key])

bokehjs/test/unit/core/util/arrayable.ts

@@ -103,4 +103,31 @@ describe("core/util/arrayable module", () => {
     const ret1 = arrayable.filter(arr1, (i) => i % 2 == 0)
     expect(ret1).to.be.equal(Float64Array.from([2, 4, 6]))
   })
+
+  it("should support min() function", () => {
+    expect(arrayable.min([])).to.be.equal(Infinity)
+    expect(arrayable.min([NaN])).to.be.equal(Infinity)
+    expect(arrayable.min([1, 2, 3])).to.be.equal(1)
+    expect(arrayable.min([3, 2, 1])).to.be.equal(1)
+    expect(arrayable.min([1, 2, NaN, 3])).to.be.equal(1)
+    expect(arrayable.min([3, 2, NaN, 1])).to.be.equal(1)
+  })
+
+  it("should support max() function", () => {
+    expect(arrayable.max([])).to.be.equal(-Infinity)
+    expect(arrayable.max([NaN])).to.be.equal(-Infinity)
+    expect(arrayable.max([1, 2, 3])).to.be.equal(3)
+    expect(arrayable.max([3, 2, 1])).to.be.equal(3)
+    expect(arrayable.max([1, 2, NaN, 3])).to.be.equal(3)
+    expect(arrayable.max([3, 2, NaN, 1])).to.be.equal(3)
+  })
+
+  it("should support minmax() function", () => {
+    expect(arrayable.minmax([])).to.be.equal([Infinity, -Infinity])
+    expect(arrayable.minmax([NaN])).to.be.equal([Infinity, -Infinity])
+    expect(arrayable.minmax([1, 2, 3])).to.be.equal([1, 3])
+    expect(arrayable.minmax([3, 2, 1])).to.be.equal([1, 3])
+    expect(arrayable.minmax([1, 2, NaN, 3])).to.be.equal([1, 3])
+    expect(arrayable.minmax([3, 2, NaN, 1])).to.be.equal([1, 3])
+  })
 })