josdejong / mathjs

An extensive math library for JavaScript and Node.js
https://mathjs.org
Apache License 2.0
14.44k stars 1.24k forks source link

nthroot((negative number)*0, 2) should be 0 and gives an error #1635

Closed dev-kg closed 2 years ago

dev-kg commented 5 years ago

by example if I do:

nthroot(-5*0, 2) = error

it shoud be:

nthroot(-5*0, 2) = 0

also, when we have any other negative number it equals error when it could be a complex number by example:

nthroot(-5, 2) = error

it should be:

nthroot(-5, 2) = 2.236067977 i

the sqrt function works properly with those cases, maybe you could replace nthroot by sqrt when the root is 2, but the problem is that if we have any even root, the same problem occurs

ericman314 commented 5 years ago

I'm not able to reproduce this. For me, I get:

nthRoot(-5*0, 2) = 0

Some ideas:

  1. Is your config using something other than regular numbers, like BigNumber?
  2. Perhaps you meant nthRoot with a capital R?

As for the second case:

nthRoot(-5, 2) = error

nthRoot always returns a real number by design; there's a good discussion of it in #851. Here are some alternatives to get the complex roots:

  1. pow(-5, 1/2), which returns the principal root (the root with the smallest "angle")
  2. nthRoots(-5, 2), which returns an array of all the roots
dev-kg commented 5 years ago

Yes the configuration is on bigNumbers

nwiatrek commented 5 years ago

For the first case, ie nthRoot(-5*0,2) = error. I created a test that shows the same as was what @ericman314 got, and even setup the configuration to be on bigNumbers:

import * as math from 'mathjs';
import { create, all } from 'mathjs';

beforeAll(() => {
    const mathjs = create(all);
    mathjs.config({ number: 'BigNumber' });
});

it('nthRoot does what it should', () => {
    expect(math.nthRoot(-5 * 0, 2)).toBe(0);
});

This returns back that the test indeed passes. I doubt that the problem is with the capital R in nthRoot since if you don't have the R an error is thrown saying that it is not a function.

What version of the package are you on?

josdejong commented 5 years ago

@nwiatrek I don't think the beforeAll in your test is doing anything: you're not using the created mathjs and also not using the expression parser which would parsed the numbers as BigNumbers.

nwiatrek commented 5 years ago

you are correct, my apologies. is this something that is more correct:

it('nthRoot does what it should', () => {
    const mathjs = create(all);
    mathjs.config({ number: 'BigNumber' });
    expect(mathjs.evaluate(mathjs.nthRoot(-5 * 0, 2))['e']).toBe(0);
});
josdejong commented 5 years ago

@nwiatrek you'll have to pass a string to mathjs.evaluate in order to create BigNumbers from values in the expression.

I can reproduce the case like this (node.js):

const { create, all } = require('.')
const mathNumber = create(all)

console.log(mathNumber.evaluate('nthRoot(-5 * 0, 2)'))
// 0 (as expected)

const mathBigNumber = create(all, { number: 'BigNumber' })
console.log(mathBigNumber.evaluate('nthRoot(-5 * 0, 2)'))
// Error: Root must be odd when a is negative.

The reason that the error is thrown in case of BigNumbers is that the current implementation sees -0 as a negative number in case of a BigNumber, but not in case of a number. We we improve that edge case for BigNumbers.

@KevinGervais would fixing this edge case address your issue?

josdejong commented 2 years ago

Closing this issue because if inactivity.