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

Median hangs if values are NaN #1214

Closed
gnobre opened this Issue Aug 20, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@gnobre

gnobre commented Aug 20, 2018

Median doesn't ever return in some cases containing NaN values, handing the process indefinitely

Hangs:

  • mathjs.median(NaN, NaN, NaN)
  • mathjs.median(2, NaN, NaN)

Returns NaN

  • mathjs.median(2, NaN)
  • mathjs.median(NaN)

I would expect all these cases to return NaN, or even something else but not to hang the javascript process.

@gnobre gnobre changed the title from Median hangs if all values are NaN to Median hangs if values are NaN Aug 20, 2018

@harrysarson harrysarson added the bug label Aug 20, 2018

@josdejong

This comment has been minimized.

Show comment
Hide comment
@josdejong

josdejong Aug 20, 2018

Owner

Thanks for reporting this bug! We'll fix it asap.

Owner

josdejong commented Aug 20, 2018

Thanks for reporting this bug! We'll fix it asap.

@josdejong josdejong closed this in b6ab40c Aug 21, 2018

@josdejong

This comment has been minimized.

Show comment
Hide comment
@josdejong

josdejong Aug 21, 2018

Owner

@gnobre this issue should be solved now in mathjs@5.1.1. Turns out there was an issue in sorting an array containing numbers including NaN: since both 2 > NaN and NaN > 2 returns false, the sorthing mechanism (inside the function partitionSelect) could get stuck in an infinite loop.

I've added tests for NaN in all statistics functions, see commit.

@harrysarson I was really happy (again) with our testing on travis: turns out assert.strictEqual(NaN, NaN) doesn't work property on older node.js versions (6 and 8), and maybe also not in browsers. 👍

Owner

josdejong commented Aug 21, 2018

@gnobre this issue should be solved now in mathjs@5.1.1. Turns out there was an issue in sorting an array containing numbers including NaN: since both 2 > NaN and NaN > 2 returns false, the sorthing mechanism (inside the function partitionSelect) could get stuck in an infinite loop.

I've added tests for NaN in all statistics functions, see commit.

@harrysarson I was really happy (again) with our testing on travis: turns out assert.strictEqual(NaN, NaN) doesn't work property on older node.js versions (6 and 8), and maybe also not in browsers. 👍

@gnobre

This comment has been minimized.

Show comment
Hide comment
@gnobre

gnobre Aug 21, 2018

@josdejong thanks for resolving this so quick, this was a really tricky one indeed!

gnobre commented Aug 21, 2018

@josdejong thanks for resolving this so quick, this was a really tricky one indeed!

@harrysarson

This comment has been minimized.

Show comment
Hide comment
@harrysarson

harrysarson Aug 21, 2018

Collaborator

The new tests are nice aswell 👍

Collaborator

harrysarson commented Aug 21, 2018

The new tests are nice aswell 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment