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

Unit.equals throws an exception when using Fractions, whilst math.equal works as expected #2918

Closed
josdejong opened this issue Mar 17, 2023 Discussed in #2915 · 6 comments

Comments

@josdejong
Copy link
Owner

From #2915

I parameterize the mathjs like this :

const config: math.ConfigOptions = {
  number: "Fraction",
};
const math: MathJsStatic = create(all, config);

I create units like this :

math.createUnit('rotation', {definition: '360 deg', aliases: ['tour','tr']})

I try to do this :

const a = math.unit(360, "deg");
const b = math.unit(1, "rotation");
expect(b.equals(a));

And i have this error :

TypeError: Cannot implicitly convert a number to a Fraction when there will be a loss of precision (value: 6.283185307179586). Use function fraction(x) to convert to Fraction.

Any idee ??!
tks.

Apparently the method Unit.equals internally tries to convert the Fraction into a number. For some reason the function math.equal(a, b) does work as expected whereas b.equals(a) throws this error.

It sounds like this is a solvable bug. Anyone able to look into this? Help would be welcome.

@brunoSnoww
Copy link
Contributor

brunoSnoww commented Mar 17, 2023

okay, so after some debugging I think the problem comes from precision when converting the unitary degree to a fraction. I've come up with the following:

Because we're using the Fraction parametrisation, at the time we compare such values, we end up in the following piece of code ( inside typed.js ):

  from: 'number',
  to: 'Fraction',
  convert: function (x) {
    if (!Fraction) {
      throwNoFraction(x)
    }
   const f = new Fraction(x)
   if (f.valueOf() !== x) {
      throw new TypeError('Cannot implicitly convert a number to a Fraction when there will be a loss of precision ' +
        '(value: ' + x + '). ' +
        'Use function fraction(x) to convert to Fraction.')
    }
    return f
 }

With 'f' and 'x' given by:

Screenshot 2023-03-17 at 18 52 59

So the condition

if(f.valueOf() !== x){
   ...
}

evaluates to true because f.valueOf() and x still differ by a small margin, and therefore the mentioned error is thrown. But even if this expression evaluates to false, we end up in this final comparison inside equalScalar.js

 'Fraction, Fraction': function (x, y) {
    return x.equals(y)
 },

Where the 'equals' function used comes from this line from fraction.js. And when the such function is called with the below arguments, it returns false:

Screenshot 2023-03-17 at 19 01 51

What I came up locally to get the right result is to instead use an equality based on if the modulo of difference is less than the default epsilon. Do we use this solutions in order to get it right ? Any thoughts ?

@josdejong
Copy link
Owner Author

Testing this again I now get the same error in both cases a.equals(b) and math.equal(a, b), that makes much more sense at least.

The cause is indeed the error that warns about losing precision. The error is thrown because we try to compare a Fraction with a number and there is indeed an inaccuracy when converting the number to a Fraction.

I think we can change the strict test if (f.valueOf() !== x) into for example if (!nearlyEqual(f.valueOf(), x, epsilon)). That way we use the same logic as in other parts of the code base. What do you think Bruno?

@maverick56
Copy link

I don't understand why we compare a fraction with a number...
In this example, both are fraction. isn't it ?

I'm surpise we need to use an epsilon to compare two fractions ... they are exact numbers, comparison should be exact. I guess.

@josdejong
Copy link
Owner Author

The reason is that in the following code the unit deg is created with a value being a number (since deg is defined as a number internally), and units rotation are created as a Fraction since they have a value defined as Fraction since mathjs was configured with fractions at the moment the unit was created.

const math = create(all, { number: 'Fraction' })
math.createUnit('rotation', {definition: '360 deg', aliases: ['tour','tr']})

const a = math.unit(360, 'deg');    // a has a value being a number since deg is defined as number
const b = math.unit(1, 'rotation'); // b has a value being a Fraction since rotation is defined as Fraction
expect(b.equals(a));

@maverick56
Copy link

OK thanks, this is clear !

josdejong added a commit that referenced this issue Apr 3, 2023
…port for units, see #2918 (#2926)

* feat: extend function `fraction` with support for units (see #2918)

* fix: update the TypeScript definitions with the new `math.fraction(value: Unit)` support

* feat: implement `math.bignumber(value: Unit)`

* feat: update type definitions of function `math.bignumber`

* fix: linting issue

* feat: implement support for `math.number(unit)` (was formerly throwing an exception)
@josdejong
Copy link
Owner Author

This is solved now by extending the function math.fraction, math.bignumber, and math.number with support for units. So, if there is a conflict, you can solve it yourself. Also, the docs now have a section explaining this in more detail.

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

No branches or pull requests

3 participants