a = b.mul(c) alters b

[josdejong]

When I do an operation on a Fraction, the original Fraction is altered, for example:

var a = Fraction(0.5);
var b = Fraction(0.4);
var c = a.mul(b);

// a = 0.2 (!), b = 0.4, c = 0.2

// ...same with other operations

Is this intentional behavior?

[infusion]

Yes, this behavior is intentional and absolutely desirable, because of method chaining:
new Fraction(2,3).mul(3).mod('1.(2)'); // 7/9

Does that harm any of your design decisions? I embed fraction.js quite often (as you can see in the samples folder) and see it as a assembly-like way of handling rational numbers. So the object represents a register if you want, which gets modified by the operations.

In your example c and a reference the same object.

[josdejong]

This is not needed for method chaining (in case of c = a.mul(b), c will get assigned the result returned by mul(b), which doesn't need to be the same as a).

It could be a design choice though, and depends on the audience you have in mind. Most libraries I've come across (like for complex numbers, bignumbers, etc) keep instances immutable. This makes it very easy to reason about your code. The only counter example I've seen is moment.js, which mutates the dates themselves when operating on them.

Now that I know it I can reckon with it. But it gives tricky side effects if you're not aware of them! It means that things go wrong by default, unless you clone the right variables at the right place.

I came across it when I implemented a method cube like:

function cube(x) {
  return x.mul(x).mul(x);   // ooops: that gives wrong results as x is mutated...
}

What I do now is put a clone() operation before every operation, and use tmp variables everywhere, so cube becomes something like:

function cube(x) {
  var tmp = x.clone();
  tmp.mul(x);
  tmp.mul(x);
  return tmp;
}

[infusion]

You're right. From that perspective and with your cube example, my design decision isn't that good. I'll think about that for 2.0.0 and hope that doesn't make things too complicated for you. For now you can come up with something shorter like:

function cube(x) {
   return x.clone().mul(x).mul(x);
}

If you have performance in mind, then something like this would speed things up:

function cube(x) {
   // Works as (n/d)^3 = n^3/d^3 and as gcd(n, d)=1 => gcd(n^3, d^3)=1 (no canceling is needed)
   x.n = Math.pow(x.n, 3);
   x.d = Math.pow(x.d, 3);
   // x.s isn't changed with odd exponent
   return x;
}

I wanted to implement a general pow() method this way, but numbers break out of the rational scope if exponents become rational as well: (2/1)^(1/2) for example.

Robert

[josdejong]

You are right I can simplify the cube method, no need for the tmp variable, much better :).

Thanks for wanting to reconsider the default behavior of fraction.js. It's no showstopper at all, I just have to consistently use clone() everywhere (and make sure I add unit tests to check whether the original input variables are not altered).

Built in support for pow would be very welcome!. Did you do performance benchmarks to validate that Math.pow(x, 3) is faster than x * x * x? And for the performance in general, do you have any benchmarks for fraction.js? You explain in the readme that you focus on performance (which is good), but I saw some things in the code that may negatively impact the performance. Having benchmarks we could easily experiment and see whether other approaches work faster. Who knows we can make fraction.js even better :).

[infusion]

Leaving just the clone() method in the code instead of a lot of tmp stuff makes it also easier to remove the overhead, when the new fraction.js behaves differently.

Built in support for pow() could be done quite easy when exponents are integers. I can add this function if you want. As I said, rational exponents make things ugly and was the only reason I haven't done it so far.

I benchmarked a lot of such tiny things when I improved the easing functions of tween.js. For ^3 it doesn't make a big difference if you use pow(x,3) or x*x*x, other than ^5 or ^10 for example. If you really want to get the last percent out of the code, then caching the attributes to reduce var-lookups would help, too. Said this, pow(x.foo, 3) could be faster than x.foo*x.foo*x.foo and var y = x.foo; y*y*y could be even faster. But it so much depends on the engine the code is running on and engines get better every day, so that it shouldn't make a big difference.

fraction.js already is quite fast. There is a jsperf ( http://jsperf.com/convert-a-rational-number-to-a-babylonian-fractions/26 ), competing against ratio.js, rational.js and some other implementations. As far as I can say, the arguments approach I use to make the API a little nicer and that I want to keep a canceled fraction all the time are the only things making the lib slower compared to a really static array approach (like the one which is faster than fraction.js).

But improvements and especially performance improvements are always welcome! So let's make it even better :)

[josdejong]

Cool to see fraction.js outperforms the alternatives, congrats :). The thing I saw in your code multiple times is calling parse(arguments). Passing arguments to another function typically disables optimization for the function at hand, so it's possible that there is room for improving performance there. Alternative would be to do copy arguments into an Array args using an old fashioned for loop, and then call parse(args). It requires quite some extra code and I don't know if it really makes a difference in this case, but it may be worth a shot.

[infusion]

Yep, that's what I just meant, that the optimizer isn't used, when I mentioned "arguments-approach". I saw this "problem" when I really stress-tested the lib. Not in terms of a performance drop, but as a note of d8 (or chrome debug). A thing I planned to do to circumvent this problem is this:

mul: function(a, b) {
parse(a, b);
return cancel(...);
}

However, this needs some extra code in parse() and isn't that relevant at the moment.

[josdejong]

Good idea, as you have limited arguments you could easily pass them one by one. It's indeed no critical thing, but it will be fun, you can really get serious performance jumps :).

[infusion]

Grats, you did it! I just removed the arguments usage. ;D I pushed it without a new version tag so far.

8,9% Improvement based on 5M iteration is not that bad :)

[josdejong]

Nice :)

I will await the release on npm, no need to hurry though, things work just fine right now using v1.7.0 (you can try the v2 branch of math.js if you like, see issue, docs and examples. Especially the expression parser makes it an interesting combi for fraction.js).

[infusion]

I know, I really like the way math.js interacts with fraction.js! :)

Just to let you know: I did some additional work on performance tweaks yesterday. For every method call, there were two gcd() invocations.

The other thing is that a pow function just arrived for integer exponents. It's the same implementation as we had earlier in this thread.

What I just saw: The valueOf() function shadows the toString() function when I cast the Fraction object to a string. For now on, it's necessary to explicitly call .toString(). Do you know a way to circumvent this behavior?

[josdejong]

I know, I really like the way math.js interacts with fraction.js! :)

Yes, same here :) Fraction.js offers exactly the API I need, without extra stuff, the code is very compact!

I'm not sure what problem you mean when Fraction as both a toString() and valueOf() function. Say you create a fraction f = new Fraction(1,2), then:

• f + "" and String(f) returns "0.5" (a string, uses toString behind the scenes)
• f + 2 gives 2.5 (a number, uses valueOf behind the scenes)

[infusion]

Yep, in this case it's trivially the same. But console.log("" + new Fraction(1, 3)) shows 0.33333333, which means, that toString() isn't called, whereas an explicit call to toString() returns 0.(3). I thought it must be exactly as you described, but unfortunately it isn't :/

[josdejong]

argh, looks like another of these odd inconsistencies in JavaScript! Would this be a reason for you not to implement valueOf though?

[infusion]

I looked up that one. JavaScript behaves the way we thought, but the algorithm is a little different when string + object is applied. It first tries to make it a primitive and after that it converts to the more dominant type. Only when no valueOf is provided, it tries toString in the same way. For us, it means that an implicit toString-invocation takes place in the context of a string. alert(new Fraction()) will work and also String(new Fraction()).

It doesn't mean that I would get rid of valueOf. This is a nice feature, but one has to know this special behavior (or better JS foo). I'll add a note to the readme in the toString section.

[infusion]

Okay 1.9.0 just arrived npm. If you change the dependency to 1.9 as well, you can use pow() :)

[josdejong]

Thanks Robert!

[infusion]

I was thinking quite long now about immutable Fraction objects. Intuitively it might make much more sense to return a new object on every operation invocation. From a performance point of view I like it much more to work on the same attributes and see the object as the algebraic container, all operations are getting applied to.

I then was thinking that a parameter could control the behavior of immutability. I could abuse the cancel() function for that, so that a new object is returned whenever "this" is returned atm. However, I more and more get distanced from the idea. The lib is really simple and introducing option parameters would be an overshoot.

So either changing the API or enhancing the documentation. I prefer the latter and leave it to the user when a new object is needed, especially because in one other project which will be published soon, I make use of the mutable behavior.

What do you think? The change would't be noteworthy, it's really just a design decision. But calling clone() whenever a new object is needed within a call-chain won't hurt :> Hopefully.

[josdejong]

I totally agree with you that you shouldn't support both behaviors. If you leave the behavior as it is (Fractions are mutated on operations) extra documentation will help a lot. If you want to make a more well-found decision here, you could do (a) benchmarks to see how much you really gain from this, and (b) collect a set of typical use-cases of Fraction.js, and see how much it helps or hurts to go for one or the other solution.

Btw what is the reason that you don't use a regular JavaScript prototype but for every operation copy variables the input variables to a global object (via parse), and at the end clean up stuff via this cancel function? It looks quite devious and error-phone if you ask me.

[infusion]

The design of the lib has two reasons: I have two pre-allocated objects in the global scope for performance reasons: One where all the parsed data goes to and one which holds the actual fraction. Not using prototypes has the reason to have "truly private methods" within the object, which are not visible to the outside world.
Okay, I could add a self executing closure function around everything, but then I have to handle the whole export mess, when used on the client and server side. This works quite okay as long as I don't want to have the Fraction function just in the current scope.
Writing libs with prototypes is one design pattern, and with this lib, I decided against it. The parse&cancel handling is quite clean if you ask me: Handle what comes in (parse), do the calc and normalize the output (cancel).

Yep, I'll do some more experiments on how the current mutable implementation outperforms one which always works on fresh objects. I think I'll wait until I finished the work on a different lib I want to publish, which relies on fraction.js, if the one or the other way is better.

I'll extend the doc for now :) && Thanks for the feedback

[josdejong]

Ok thanks for the explanation. It's funny that in a couple of areas we would make the exact opposite decisions. But well, Fraction.js has a nice API, works fast, and is well tested, so I'm happy with it :).

[infusion]

I think it's a mix of a functional and OOP approach if you will but things aren't cast in stone. If you find time, I'd be glad to hear what opposite decisions you would have made.

[josdejong]

Some random thoughts:

• I don't mind so much having "private" functions/properties publicly accessible (I often prefix them with an underscore to let the developer know that it's not the public API).
• I would put the code in a regular UMD closure.
• I would start writing this library as a regular JS prototype. Browsers are heavily optimized for this and people easily understand what's going on as it's a familiar pattern. So I would expect that to work out nicely.
• Where possible I would use pure functions (functional approach), right now you have exactly the opposite as almost all functions depend on a number of gobally shared variables which (= impure functions, side effects, more complicated to reason about, more error prone).
• I haven't tested, but I was wondering whether it isn't expensive to have all your (private) functions in the body of the Fraction constructor function. Doesn't that mean that with every new Fraction(...) the complete body of your library is evaluated again and again?
• They say the use of self is a code smell.

Not meant to criticize how you set up the library but meant as constructive input, some food for thought.

[infusion]

Okay, finally published 2.0.0 with immutable objects, a prototype design, removal of self, UMD closure and some minor optimizations. The API is still the same (without the set method as this doesn't make sense for immutable objects anymore).

Thanks for your input!

[josdejong]

wouw, thanks, that looks very clean :) Thanks for rethinking immutability of your lib.