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

rationalize() throws exception for some input with decimals #1146

Closed
maruta opened this Issue Jun 26, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@maruta

maruta commented Jun 26, 2018

rationalize() seems to fail for some input with decimals.
So far, I tested
math.rationalize('1/(0.1x+1)+1').toString()
-> throws exception
math.rationalize('1/(1.0x+1)+1').toString()
-> "(x + 2) / (x + 1)" OK
math.rationalize('1/(0.1x+1)').toString()
-> "10 / (x + 10)" OK
with version 5.0.0.

Thanks!

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 27, 2018

Hm, that's odd. The error you get with decimals is:

math.rationalize('1/(0.1x+1)+1').toString())
// ArgumentsError: Wrong number of arguments in 
// function Operator / invalid (undefined provided, undefined expected)

@paulobuchsbaum do you have an idea where this can originate?

@josdejong josdejong added the bug label Jun 27, 2018

@maruta

This comment has been minimized.

maruta commented Jun 28, 2018

After further investigation,
I found a part of the problem is related to negative integer exponents.
This problem is reproduced by simpler example

math.rationalize('1/x^2+1')
// ArgumentsError: There is a non-integer exponent

and some modification seems to solve this problem.
maruta@35c9aa7

To make

math.rationalize('1/(0.1x+1)+1').toString())

work, I needed to modify rationalize() algorithm
maruta@b0d077f
but I'm not confident this is preferable in general case.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jun 28, 2018

When you run npm test after your changes, do they all pass?

@maruta

This comment has been minimized.

maruta commented Jun 28, 2018

When I run npm test in my environment, one test fails as follows

  4166 passing (35s)
  18 pending
  1 failing

  1) Unit
       prefixes
         should accept both long and short prefixes for bar:
     SyntaxError: Unit "millibar" not found.

the failed test also fails in v5.0.0 and seems to have nothing to do with my change.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 1, 2018

Thanks for diving into this @maruta! I hope to look into this soon unless @paulobuchsbaum beats me ;)

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 4, 2018

I've implemented a fix based on the work of @maruta (thanks again!), see 0d93fff

@paulobuchsbaum and @maruta can you please review this commit? I'm not very familiar/confident with this part of the code base, so a pair of extra eyes would be helpful.

@josdejong josdejong closed this in 0d93fff Jul 7, 2018

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 7, 2018

I've released the fix based on @maruta 's work in v5.0.2, please let me know if there are still issues in this regard.

@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Jul 11, 2018

I was traveling without reading my emails, but I'm back now.

I think that the first change from @maruta
(maruta/mathjs@35c9aa7) contains an error.

The new condtion needs to be made with unaryminus operator and not using general - operator. With rationalize polynomials function I did not intend to allow general expressions in exponent.

For accepting negative exponents it's only necessary only add 3 lines based on 3.19 version (my last version). It's not important but it's redundant using isbinary test in ^ operator.

      else if (tp==='OperatorNode')  {
        if (node.op==='^')  {
        // New code - 3 lines added on 3.19 version
          if (node.args[1].fn === 'unaryMinus'){ 
             node = node.args[0]                             
          }                                                                
          if (node.args[1].type!=='ConstantNode' || 
                    ! number.isInteger(parseFloat(node.args[1].value)))
             throw new ArgumentsError('There is a non-integer exponent');
          else
             recPoly(node.args[0]);      
      }

The second change (related to the simplify rules) is in maruta/mathjs@b0d077f

It solves its problem but adds new ones to the code, concerning tougher expressions (that was extracted from tests because was slow), because changes the rules applications procedures.

The right and simpler approach is put an option in simplifyconstant rule allowing to allow turning off exact fraction conversion. Other option, less elegant, is cloning simplifyconstant in another module without exact fraction.

I also have added in my sets of rules 2 new rules to handle with v/c parts in expressions, so X/10 turns (1/10)*x that can be considered as a coefficient.

I've tested in my local machine and the below expressions work smoothly. For testing in my local simplifyconstant, I have commented out the lines in _exactFraction function, but obviously in real code one should create an option for using or not exact fraction conversion.

1/(0.1x+1)+1
1/x^2+1
1/(x/10+1)+1

@josdejong, can I make a new pull request? What branch?

@maruta

This comment has been minimized.

maruta commented Jul 12, 2018

@josdejong @paulobuchsbaum
Thanks a lot!
I totally agree with comments by @paulobuchsbaum

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 14, 2018

Thanks for your input @paulobuchsbaum . I don't fully understand all your points but if you can turn it into a PR it would be great!

@josdejong, can I make a new pull request? What branch?

To the develop branch. Note that there has been quite some refactoring going in v4 and v5, so please make sure your develop branch is up to date before implementing your suggested improvements.

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