Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use utility isNaN for consistent max and min results #3389

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/expression/embeddedDocs/function/relational/larger.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export const largerDocs = {
'larger(x, y)'
],
description:
'Check if value x is larger than y. Returns true if x is larger than y, and false if not.',
'Check if value x is larger than y. Returns true if x is larger than y, and false if not. Comparing a value with NaN returns false.',
examples: [
'2 > 3',
'5 > 2*2',
Expand Down
2 changes: 1 addition & 1 deletion src/expression/embeddedDocs/function/relational/smaller.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export const smallerDocs = {
'smaller(x, y)'
],
description:
'Check if value x is smaller than value y. Returns true if x is smaller than y, and false if not.',
'Check if value x is smaller than value y. Returns true if x is smaller than y, and false if not. Comparing a value with NaN returns false.',
examples: [
'2 < 3',
'5 < 2*2',
Expand Down
2 changes: 1 addition & 1 deletion src/expression/embeddedDocs/function/statistics/max.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export const maxDocs = {
'max(A)',
'max(A, dimension)'
],
description: 'Compute the maximum value of a list of values.',
description: 'Compute the maximum value of a list of values. If any NaN values are found, the function yields the last NaN in the input.',
examples: [
'max(2, 3, 4, 1)',
'max([2, 3, 4, 1])',
Expand Down
2 changes: 1 addition & 1 deletion src/expression/embeddedDocs/function/statistics/min.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export const minDocs = {
'min(A)',
'min(A, dimension)'
],
description: 'Compute the minimum value of a list of values.',
description: 'Compute the minimum value of a list of values. If any NaN values are found, the function yields the last NaN in the input.',
examples: [
'min(2, 3, 4, 1)',
'min([2, 3, 4, 1])',
Expand Down
6 changes: 3 additions & 3 deletions src/expression/transform/max.transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import { createMax } from '../../function/statistics/max.js'
import { lastDimToZeroBase } from './utils/lastDimToZeroBase.js'

const name = 'max'
const dependencies = ['typed', 'config', 'numeric', 'larger']
const dependencies = ['typed', 'config', 'numeric', 'larger', 'isNaN']

export const createMaxTransform = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, larger }) => {
const max = createMax({ typed, config, numeric, larger })
export const createMaxTransform = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, larger, isNaN }) => {
const max = createMax({ typed, config, numeric, larger, isNaN })

/**
* Attach a transform function to math.max
Expand Down
6 changes: 3 additions & 3 deletions src/expression/transform/min.transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import { createMin } from '../../function/statistics/min.js'
import { lastDimToZeroBase } from './utils/lastDimToZeroBase.js'

const name = 'min'
const dependencies = ['typed', 'config', 'numeric', 'smaller']
const dependencies = ['typed', 'config', 'numeric', 'smaller', 'isNaN']

export const createMinTransform = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, smaller }) => {
const min = createMin({ typed, config, numeric, smaller })
export const createMinTransform = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, smaller, isNaN }) => {
const min = createMin({ typed, config, numeric, smaller, isNaN })

/**
* Attach a transform function to math.min
Expand Down
4 changes: 2 additions & 2 deletions src/function/matrix/partitionSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { factory } from '../../utils/factory.js'
const name = 'partitionSelect'
const dependencies = ['typed', 'isNumeric', 'isNaN', 'compare']

export const createPartitionSelect = /* #__PURE__ */ factory(name, dependencies, ({ typed, isNumeric, isNaN, compare }) => {
export const createPartitionSelect = /* #__PURE__ */ factory(name, dependencies, ({ typed, isNumeric, isNaN: mathIsNaN, compare }) => {
const asc = compare
const desc = (a, b) => -compare(a, b)

Expand Down Expand Up @@ -99,7 +99,7 @@ export const createPartitionSelect = /* #__PURE__ */ factory(name, dependencies,

// check for NaN values since these can cause an infinite while loop
for (let i = 0; i < arr.length; i++) {
if (isNumeric(arr[i]) && isNaN(arr[i])) {
if (isNumeric(arr[i]) && mathIsNaN(arr[i])) {
return arr[i] // return NaN
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/function/statistics/max.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import { safeNumberType } from '../../utils/number.js'
import { improveErrorMessage } from './utils/improveErrorMessage.js'

const name = 'max'
const dependencies = ['typed', 'config', 'numeric', 'larger']
const dependencies = ['typed', 'config', 'numeric', 'larger', 'isNaN']

export const createMax = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, larger }) => {
export const createMax = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, larger, isNaN: mathIsNaN }) => {
/**
* Compute the maximum value of a matrix or a list with values.
* In case of a multidimensional array, the maximum of the flattened array
Expand Down Expand Up @@ -83,8 +83,8 @@ export const createMax = /* #__PURE__ */ factory(name, dependencies, ({ typed, c

deepForEach(array, function (value) {
try {
if (typeof value === 'number' && isNaN(value)) {
res = NaN
if (mathIsNaN(value)) {
res = value
} else if (res === undefined || larger(value, res)) {
res = value
}
Expand Down
8 changes: 4 additions & 4 deletions src/function/statistics/min.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import { safeNumberType } from '../../utils/number.js'
import { improveErrorMessage } from './utils/improveErrorMessage.js'

const name = 'min'
const dependencies = ['typed', 'config', 'numeric', 'smaller']
const dependencies = ['typed', 'config', 'numeric', 'smaller', 'isNaN']

export const createMin = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, smaller }) => {
export const createMin = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, numeric, smaller, isNaN: mathIsNaN }) => {
/**
* Compute the minimum value of a matrix or a list of values.
* In case of a multidimensional array, the minimum of the flattened array
Expand Down Expand Up @@ -83,8 +83,8 @@ export const createMin = /* #__PURE__ */ factory(name, dependencies, ({ typed, c

deepForEach(array, function (value) {
try {
if (typeof value === 'number' && isNaN(value)) {
min = NaN
if (mathIsNaN(value)) {
min = value
} else if (min === undefined || smaller(value, min)) {
min = value
}
Expand Down
4 changes: 2 additions & 2 deletions src/function/statistics/mode.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { factory } from '../../utils/factory.js'
const name = 'mode'
const dependencies = ['typed', 'isNaN', 'isNumeric']

export const createMode = /* #__PURE__ */ factory(name, dependencies, ({ typed, isNaN, isNumeric }) => {
export const createMode = /* #__PURE__ */ factory(name, dependencies, ({ typed, isNaN: mathIsNaN, isNumeric }) => {
/**
* Computes the mode of a set of numbers or a list with values(numbers or characters).
* If there are multiple modes, it returns a list of those values.
Expand Down Expand Up @@ -57,7 +57,7 @@ export const createMode = /* #__PURE__ */ factory(name, dependencies, ({ typed,
for (let i = 0; i < values.length; i++) {
const value = values[i]

if (isNumeric(value) && isNaN(value)) {
if (isNumeric(value) && mathIsNaN(value)) {
throw new Error('Cannot calculate mode of an array containing NaN values')
}

Expand Down
4 changes: 2 additions & 2 deletions src/function/statistics/variance.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const DEFAULT_NORMALIZATION = 'unbiased'
const name = 'variance'
const dependencies = ['typed', 'add', 'subtract', 'multiply', 'divide', 'mapSlices', 'isNaN']

export const createVariance = /* #__PURE__ */ factory(name, dependencies, ({ typed, add, subtract, multiply, divide, mapSlices, isNaN }) => {
export const createVariance = /* #__PURE__ */ factory(name, dependencies, ({ typed, add, subtract, multiply, divide, mapSlices, isNaN: mathIsNaN }) => {
/**
* Compute the variance of a matrix or a list with values.
* In case of a multidimensional array or matrix, the variance over all
Expand Down Expand Up @@ -124,7 +124,7 @@ export const createVariance = /* #__PURE__ */ factory(name, dependencies, ({ typ
sum = sum === undefined ? multiply(diff, diff) : add(sum, multiply(diff, diff))
})

if (isNaN(sum)) {
if (mathIsNaN(sum)) {
return sum
}

Expand Down
27 changes: 27 additions & 0 deletions test/unit-tests/function/relational/larger.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,33 @@ describe('larger', function () {
assert.throws(function () { larger('A', 'B') }, /Cannot convert "A" to a number/)
})

it('should result in false when comparing a something with NaN', function () {
// Number
assert.strictEqual(larger(NaN, 3), false)
assert.strictEqual(larger(3, NaN), false)
assert.strictEqual(larger(NaN, NaN), false)

// BigNumber
assert.strictEqual(larger(NaN, bignumber(3)), false)
assert.strictEqual(larger(bignumber(3), NaN), false)
assert.strictEqual(larger(3, bignumber(NaN)), false)
assert.strictEqual(larger(bignumber(NaN), 3), false)
assert.strictEqual(larger(bignumber(NaN), bignumber(3)), false)
assert.strictEqual(larger(bignumber(3), bignumber(NaN)), false)

// Fraction
assert.strictEqual(larger(NaN, fraction(3)), false)
assert.strictEqual(larger(fraction(3), NaN), false)
assert.strictEqual(larger(fraction(3), bignumber(NaN)), false)
assert.strictEqual(larger(bignumber(NaN), fraction(3)), false)
// A fraction itself will throw an error when it's NaN

// Unit
assert.strictEqual(larger(unit('3', 's'), unit(NaN, 's')), false)
assert.strictEqual(larger(unit(NaN, 's'), unit('3', 's')), false)
assert.strictEqual(larger(unit(NaN, 's'), unit(NaN, 's')), false)
})

describe('Array', function () {
it('should compare array - scalar', function () {
assert.deepStrictEqual(larger(2, [1, 2, 3]), [true, false, false])
Expand Down
27 changes: 27 additions & 0 deletions test/unit-tests/function/relational/smaller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,33 @@ describe('smaller', function () {
assert.throws(function () { smaller('A', 'B') }, /Cannot convert "A" to a number/)
})

it('should result in false when comparing a something with NaN', function () {
// Number
assert.strictEqual(smaller(NaN, 3), false)
assert.strictEqual(smaller(3, NaN), false)
assert.strictEqual(smaller(NaN, NaN), false)

// BigNumber
assert.strictEqual(smaller(NaN, bignumber(3)), false)
assert.strictEqual(smaller(bignumber(3), NaN), false)
assert.strictEqual(smaller(3, bignumber(NaN)), false)
assert.strictEqual(smaller(bignumber(NaN), 3), false)
assert.strictEqual(smaller(bignumber(NaN), bignumber(3)), false)
assert.strictEqual(smaller(bignumber(3), bignumber(NaN)), false)

// Fraction
assert.strictEqual(smaller(NaN, fraction(3)), false)
assert.strictEqual(smaller(fraction(3), NaN), false)
assert.strictEqual(smaller(fraction(3), bignumber(NaN)), false)
assert.strictEqual(smaller(bignumber(NaN), fraction(3)), false)
// A fraction itself will throw an error when it's NaN

// Unit
assert.strictEqual(smaller(unit('3', 's'), unit(NaN, 's')), false)
assert.strictEqual(smaller(unit(NaN, 's'), unit('3', 's')), false)
assert.strictEqual(smaller(unit(NaN, 's'), unit(NaN, 's')), false)
})

describe('Array', function () {
it('should compare array - scalar', function () {
assert.deepStrictEqual(smaller(2, [1, 2, 3]), [false, false, true])
Expand Down
10 changes: 8 additions & 2 deletions test/unit-tests/function/statistics/max.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const Complex = math.Complex
const DenseMatrix = math.DenseMatrix
const fraction = math.fraction
const max = math.max
const unit = math.unit

describe('max', function () {
it('should return the max of numbers', function () {
Expand Down Expand Up @@ -89,6 +90,11 @@ describe('max', function () {
assert(isNaN(max([1, 3, NaN])))
assert(isNaN(max([NaN, NaN, NaN])))
assert(isNaN(max(NaN, NaN)))
assert(isNaN(max(BigNumber(NaN), BigNumber(123))))
assert(isNaN((max(BigNumber(123), BigNumber(NaN), NaN))))
assert(isNaN(max(unit(NaN, 's'), unit(123, 's')).value))
assert(isNaN(max(unit(123, 's'), unit(NaN, 's')).value))
assert(isNaN(max(1, 3, fraction(2, 3), fraction(1, 2), NaN, BigNumber(1), BigNumber(NaN), 5, Infinity, -Infinity)))
})

it('should return the largest of mixed types', function () {
Expand Down Expand Up @@ -124,8 +130,8 @@ describe('max', function () {
assert.throws(function () { max([[2, new Date(), 4]]) }, /TypeError: Cannot calculate max, unexpected type of argument/)
assert.throws(function () { max([2, null, 4]) }, /TypeError: Cannot calculate max, unexpected type of argument/)
assert.throws(function () { max([[2, 5], [4, null], [1, 7]], 0) }, /TypeError: Cannot calculate max, unexpected type of argument/)
assert.throws(function () { max('a', 'b') }, /Error: Cannot convert "b" to a number/)
assert.throws(function () { max('a') }, /SyntaxError: String "a" is not a valid number/)
assert.throws(function () { max('a', 'b') }, /Error: Cannot convert "a" to a number/)
assert.throws(function () { max('a') }, /Error: Cannot convert "a" to a number/)
})

it('should return undefined if called with an empty array', function () {
Expand Down
10 changes: 8 additions & 2 deletions test/unit-tests/function/statistics/min.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const Complex = math.Complex
const DenseMatrix = math.DenseMatrix
const fraction = math.fraction
const min = math.min
const unit = math.unit

describe('min', function () {
it('should return the min between several numbers', function () {
Expand Down Expand Up @@ -98,6 +99,11 @@ describe('min', function () {
assert(isNaN(min([1, 3, NaN])))
assert(isNaN(min([NaN, NaN, NaN])))
assert(isNaN(min(NaN, NaN, NaN)))
assert(isNaN(min(BigNumber(NaN), BigNumber(123))))
assert(isNaN((min(BigNumber(123), BigNumber(NaN), NaN))))
assert(isNaN(min(unit(NaN, 's'), unit(123, 's')).value))
assert(isNaN(min(unit(123, 's'), unit(NaN, 's')).value))
assert(isNaN(min(1, 3, fraction(2, 3), fraction(1, 2), NaN, BigNumber(1), BigNumber(NaN), 5, Infinity, -Infinity)))
})

it('should return the smallest of mixed types', function () {
Expand Down Expand Up @@ -137,8 +143,8 @@ describe('min', function () {
assert.throws(function () { min([[2, new Date(), 4]]) }, /TypeError: Cannot calculate min, unexpected type of argument/)
assert.throws(function () { min([2, null, 4]) }, /TypeError: Cannot calculate min, unexpected type of argument/)
assert.throws(function () { min([[2, 5], [4, null], [1, 7]], 0) }, /TypeError: Cannot calculate min, unexpected type of argument/)
assert.throws(function () { min('a', 'b') }, /Error: Cannot convert "b" to a number/)
assert.throws(function () { min('a') }, /SyntaxError: String "a" is not a valid number/)
assert.throws(function () { min('a', 'b') }, /Error: Cannot convert "a" to a number/)
assert.throws(function () { min('a') }, /Error: Cannot convert "a" to a number/)
})

it('should LaTeX min', function () {
Expand Down