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

fix #134, run formatter #135

Closed
wants to merge 2 commits into from
Closed

fix #134, run formatter #135

wants to merge 2 commits into from

Conversation

Dakkers
Copy link
Contributor

@Dakkers Dakkers commented Feb 12, 2015

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.

…n vector transposes; add helper functions for mat-mat, mat-vec and vec-mat multiplication
…yword; run format (apparently for the first time); add .jshintrc; update public js
@LarryBattle
Copy link
Contributor

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
Copy link
Contributor Author

Dakkers commented Feb 13, 2015

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

@Dakkers
Copy link
Contributor Author

Dakkers commented Feb 18, 2015

ping @LarryBattle

@LarryBattle
Copy link
Contributor

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

@Dakkers
Copy link
Contributor Author

Dakkers commented Feb 19, 2015

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
Copy link
Contributor Author

Dakkers commented Feb 19, 2015

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

@LarryBattle
Copy link
Contributor

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
Copy link
Contributor Author

Dakkers commented Feb 19, 2015

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
Copy link
Contributor Author

Dakkers commented Feb 19, 2015

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
Copy link
Contributor Author

Dakkers commented Feb 28, 2015

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

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

Successfully merging this pull request may close these issues.

matrix.transpose doesn't work correctly on vectors
2 participants