numbers / numbers.js

Advanced Mathematics Library for Node.js and JavaScript
Apache License 2.0
1.77k stars 167 forks source link

fix #134, run formatter #135

Closed Dakkers closed 9 years ago

Dakkers commented 9 years ago

don't be scared by the huge number of additions and deletions! the only new content is the following:

everything else is just npm run format doing its thing.

@LarryBattle @KartikTalwar it'd be cool if you could look at the 2 first things I mentioned.

LarryBattle commented 9 years ago

I did a quick run through and it looks good so far. I'll try to do another pass to double check the logic.

Great job!

Github tip: Add ?w=1 to the end of the url when comparing diffs to ignore white space. Ex for this pull request https://github.com/sjkaliski/numbers.js/pull/135/files?w=1

More Github secrets here: https://github.com/tiimgreen/github-cheat-sheet

Dakkers commented 9 years ago

ignoring whitespace in diffs makes a huge difference in these commits, holy shet

Dakkers commented 9 years ago

ping @LarryBattle

LarryBattle commented 9 years ago

The only suggestion I have right now is that matrix.transpose() should have more test cases. Otherwise it looks good.

Dakkers commented 9 years ago

transposeColumnVector should only take column vectors as arguments, not matrices. a transposed column vector should be a row vector. IMO a row vector should be like [1,0,0], not [ [1, 0, 0] ] like what is currently returned. example:

// old version
matrix.transposeColumnVector([[1], [2], [3]])
// [ [1, 2, 3] ]

// new version
matrix.transposeColumnVector([[1],[2],[3]])
// [1, 2, 3]
Dakkers commented 9 years ago

I could make transposeColumnVector and transposeRowVector throw errors if given the wrong kinds of vectors.

LarryBattle commented 9 years ago

I think the old version is just fine. transposeColumnVector and transposeRowVector really should be private methods since their sole purpose is to optimize matrix.transpose().

Dakkers commented 9 years ago

I disagree. the intuitive way of thinking of a row vector is [1,0,0] as seen in numpy (arrays) and MATLAB and most other languages, and not [ [1,0, 0] ].

if one does in fact prefer it to represent a row vector as [ [1, 0, 0] ] then my transpose function will correctly return [ [1], [0], [0] ]. it will not work the other way though unless I add in a flag that will tell it to, which is easily doable.

I agree with that to an extent. however, for a micro-optimization, one could call these methods directly instead. not sure if that's needed.

Dakkers commented 9 years ago

although, the problem with that is that other functions may not return what the person expected. perhaps a setting, like EPSILON, that would format the vectors correctly in all functions.

Dakkers commented 9 years ago

will be closing this PR for reasons I'll mention later today