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

Merge pull request #2 from josdejong/develop #1173

Merged
merged 46 commits into from Aug 3, 2018

Conversation

Projects
None yet
4 participants
@paulobuchsbaum
Contributor

paulobuchsbaum commented Jul 17, 2018

Fix of bug fixes in rationalize.js, also changing simplify.js and simplifyConstant.js and more 2 bugs in simplify.js and simplifyconstant.js in order to be possible passing in Travis test.

Bugs in simplifyConstant.js and simplify.js

  1. simplifyConstant.js - I've changed new ConstantNode(stringNumber, 'number') to new ConstantNode(number)

  2. simplify.js - Due to problems with a number node with string type, I've added !isNaN(node.value))) in number type test condition

Bugs in rationalize.js

  1. I've fixed negative power exponents and decimals coefficients troubles. The decimals coefficients problem has led to the need to add a new feature in simplify.js and simplifyConstant.js (next topic)

New feature in simplify.js and simplifyConstant.js

  1. New rule type (string), whose valid values are in listCommStrings new variable. The only string rule accepted so far is to turn off exact fraction conversion in simplifyConstant.js

paulobuchsbaum added some commits Jul 17, 2018

Combined changes
2 new simplification rules and small changes in rationalize. New test set in rationalize.test
LINT corrections
LINT corrections
@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Jul 17, 2018

My local test in rationalize.test.js using mocha is generating a weird error

SyntaxError: Unexpected token import

import core from './core/core'

I've researched in Internet and they say that Node.js does not accept import statement. However, I've tried some months ago and there was no problem! It is a kind of mistery

paulobuchsbaum added some commits Jul 17, 2018

lint corrections
lint corrections
lint corrections
lint corrections
small bug fixes
small bug fixes
small corrections
small corrections
Number type error (old)
Number type error
small fixes (Travis)
small fixes (Travis)
small lint fixes
small lint fixes
2 small fixes (Travis)
2 small fixes (Travis)
@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Jul 18, 2018

Now everything is OK. The only complain is that I don't get run Mocha in my local environment, so I should to rewrite rationalize.js just for I get debugging the code.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 19, 2018

Thanks a lot for the fixes and improvements Paulo, looks good!

I don't fully understand isExactFract. Is this meant as a public option for simplify? If so, shouldn't we pass it via an object with options like simplify(expr, ..., {exactFractions: false}) instead of "disguised" as a rule?

I've researched in Internet and they say that Node.js does not accept import statement. However, I've tried some months ago and there was no problem! It is a kind of mistery

Sorry for the inconvenience. One month ago we released v5.0.0 where we changed the code from ES5 to ES6+, and set up build scripts to compile modern JS (in src) back to safe, old JS (in lib and dist). Thanks to the babel transpilation to ES5 we can use the newest javascript features, including import/export. However this isn't yet supported by node.js, which means we have to transpile everything too before we can run it in node.js. This includes running the unit tests: we first have to transpile to ES5, then we can run it on node.js. To do this, you have to pass an extra argument --require babel-core/register to mocha (see package.json):

mocha test test-node --recursive --require babel-core/register

Or simply run npm test of course, that will also run the linter that we introduced in mathjs v5 too :)

@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Jul 19, 2018

I don't fully understand isExactFract. Is this meant as a public option for simplify? If so, shouldn't we pass it via an object with options like simplify(expr, ..., {exactFractions: false}) instead of "disguised" as a rule?

Yes, you are right, @josdejong. I'll make the changes accordingly.

paulobuchsbaum added some commits Jul 19, 2018

Add options parameter in simplify
Add a  new option parameter in simplify as an object with property exactFractions
small fixes
small fixes
lint fixes
lint fixes
lint fixes
lint fixes
lint fixes
lint fixes
small fixes
small fixes
lint fix
lint fix
small fix II
small fix II
small fix III
small fix III
more fixes
more fixes
very small fix
very small fix
@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Jul 20, 2018

Done!

Your suggestion was much better than mine, but I had to change the parameter profile in simplify.js, that was long (12 different options ).

Now there is a new parameter in simplify function that can be used to put additional options. For now, it accepts only a boolean property: exactFractions. Default value is true, but rationalize uses false.

true value in exactFractions simplify 0.1*x to x/10, however false value get 0.1*x and keeps it..
This is referring to simplifyConstant function, which is part of the simplify standard rules.

I've added some calls to simplify.test.js in order to test the changes in simplify.js

@@ -57,7 +57,7 @@ function factory (type, config, load, typed, math) {
*
* Syntax:
*
* simplify(expr)
* simplify(expr)n

This comment has been minimized.

@josdejong

josdejong Jul 22, 2018

Owner

I think the added character n is a typo?

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 22, 2018

Thanks @paulobuchsbaum , looks good like this, extending simplify with an options argument works out really nicely!

I'm not sure if you know this already, but you can run the linter locally via npm run lint, and it is also executed when running npm test.

Here a few feedback points:

  1. I was wondering about the new added rules, do they belong in rationalize, or should we move them to simplify?

    {l: 'c1*n + n', r: '(c1+1)*n'}, // Joining constants
    {l: 'c1*n - c2*n', r: '(c1-c2)*n'}, // Joining constants
    {l: 'c1*n - n', r: '(c1-1)*n'}, // Joining constants
    {l: 'v/c', r: '(1/c)*v'}, // variable/constant (new!)
    {l: 'v/-c', r: '-(1/c)*v'}, // variable/constant (new!)
    
  2. It's a minor thing, but I personally prefer variable names like options instead of short names like opts. I find it more descriptive and since we nowadays we have IDE's with autocompletion and plenty of RAM, a few bytes extra isn't a problem ;)

  3. It may be more straigtforward and easier to extend later if we simply pass options to all rules, like:

    res = rules[i](res, options)

    instead of:

    if (rules[i] === simplifyConstant) {
      res = rules[i](res, exactFract)
    } else {
      res = rules[i](res)
    }

    what do you think?

  4. Currently, the simplifyConstant handles the exactFraction option by moving it in a "global" constant (ie. defined outside of the function) when calling simplifyConstant, and indirectly using it in _toNumber. I think it will be more robust to pass it as an argument to _toNumber, so it's straigtforward to see which functions use/pass these options. And same as (3.) maybe we should simply pass options instead of exactFractions everywhere.

  5. I think we should describe this new option in the docs of simplify, and probably create two simple extra examples demonstrating what exactFractions does. If you want I can help out here too and write the docs.

  6. Thanks for working out unit tests for the new simplify options 👍 .

small changes
small changes

paulobuchsbaum added some commits Jul 31, 2018

Revert "Small fix"
This reverts commit 317aa6c.
Revert "small syntax fix"
This reverts commit 70843c9.
Revert "Syntax error fix"
This reverts commit 03527d1.
Revert "small fixes"
This reverts commit c36f316.
Revert "small fix"
This reverts commit bed26cd.
Revert "small fixes"
This reverts commit 48a07a5.
Revert "typo fix"
This reverts commit 2f0cb5d.
Revert "lint fixes"
This reverts commit e4ea989.
Revert "small changes"
This reverts commit 99b4c6a.
@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Jul 31, 2018

I try to change the code to make the options parameter local, but additionally I had to change some calls to foldFraction using map to for loops and also I try to change typed function _toNumber for a new profile with 2 parameters.

I had changed and added 31 lines (> 21 lines that I said above). I've realized that using options as a local variable passing down through tree of calls get hard to discover when the options parameter is effectively used and in any middle call (_eval(....)) it's a complete mistery the options meaning for any code reviewer.

In the end, many new errors have arisen and I've got so confused, that I've reverted all changes to previous state. I give in!

However, I think that it is very unlikely that new options will be necessary in simplifyConstant.js to the point of turning the code into spaghetti.

Another day, I will transfer the rules from rationalize.js to simplify.js default rules and propose an addendum in the documentation.

@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Jul 31, 2018

If all the auxiliary functions (_toNumber, _eval, foldFraction and _exactFraction) were internal to simplifyConstant function, its parameters could be used directly without assignment to a global variable. Even so, is this against your philosophy?

@josdejong

This comment has been minimized.

Owner

josdejong commented Aug 3, 2018

Thanks for giving it a try refactoring the options argument @paulobuchsbaum . I will merge your fixes now and give the refactoring a try too. If it indeed gives more issues then it solves I guess we should keep it as is. I will also describe options in the docs of simplify and add an example.

If all the auxiliary functions (_toNumber, _eval, foldFraction and _exactFraction) were internal to simplifyConstant function, its parameters could be used directly without assignment to a global variable. Even so, is this against your philosophy?

In mathjs we have that this situation for example with the dependency injection mechanism which passes typed and config and a some other things. In such a case I would say it's best to use the parameters that are already defined in the outer scope of the function and not pass them as argument to internal functions. I think the main reason for me would be that (a) these parameters are constants in the scope of this function, they will never ever change again unlike our current options "global", and (b) if you would pass them as arguments too, you would have two variables with the same name, shadowing each other, which is confusing. Does that sort of make sense?

@josdejong josdejong merged commit 443d42a into josdejong:develop Aug 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (josdejong) No new issues
Details

josdejong added a commit that referenced this pull request Aug 3, 2018

josdejong added a commit that referenced this pull request Aug 3, 2018

@josdejong

This comment has been minimized.

Owner

josdejong commented Aug 3, 2018

I've refactored globalOptions into options, see e296fdc. There is about 30 occurances of options now, which is a lot.

I've realized that using options as a local variable passing down through tree of calls get hard to discover when the options parameter is effectively used and in any middle call (_eval(....)) it's a complete mistery the options meaning for any code reviewer.

I actually do like the transparency that I see after the refactoring: before, you had no clue that _eval actually needed options because one of the functions it calls needs options. Now you clearly see it.

I guess it's partly personal preference and I guess we simply have different opinions here, which is ok. Thanks for the constructive discussion and your flexibility.

@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Aug 3, 2018

Great! I've got the meaning of your refactoring. I've commit an error in apply usage. Thanks for all debate.

@paulobuchsbaum paulobuchsbaum deleted the paulobuchsbaum:develop branch Aug 3, 2018

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Aug 3, 2018

Nice work here 👍 These changes look great, I agree that parsing options around will make this code much easier for others to read and will make maintainance much smoother going forwards 😄

@josdejong

This comment has been minimized.

Owner

josdejong commented Aug 12, 2018

@paulmasson your fixes and improvements are now available in v5.1.0. Thanks again for all your effort!

@paulmasson

This comment has been minimized.

Contributor

paulmasson commented Aug 12, 2018

You’d be welcome, but I think you mean @paulobuchsbaum

@josdejong

This comment has been minimized.

Owner

josdejong commented Aug 12, 2018

whoops 😳, sorry Paul. I mean @paulobuchsbaum instead

@paulobuchsbaum

This comment has been minimized.

Contributor

paulobuchsbaum commented Aug 13, 2018

☺️, thank you, @josdejong, for the opportunity to interact and learn.

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