implementing rotate(w, theta) #1992
Conversation
rnd-debug
changed the title
|
Thanks @rnd-debug ! I'll look into your PR later (can't keep up with all issues/PR's at this moment, but no worries). I'll add the |
josdejong
added
the
hacktoberfest-accepted
There was a problem hiding this comment.
Looks very good and well tested @rnd-debug, thanks!
I made two small comments, one about making sure the behavior with data types is consistent with other functions.
src/function/matrix/rotate.js
Outdated
| 'Array , number | BigNumber | Complex | Unit': function (w, theta) { | ||
| _validateArraySize(w, 2) | ||
| const matrixRes = multiply(rotationMatrix(theta), w) | ||
| return config.predictable ? matrixRes.toArray() : matrixRes |
There was a problem hiding this comment.
So far, in mathjs the behavior of functions is as follows:
- array in -> array out
- matrix in -> matrix out
- when array/matrix output cannot be determined from input, look at
config.matrix(for exampleones(3)) - when the output would switch to a different type, like
sqrt(-4)returning a complex number2i, look atconfig.predictable, when false, return a numberNaNinstead of changing output to a complex number.
I think here it will be good to always return an Array when the input was an Array, and the same for matrices. Does that make sense to you?
There was a problem hiding this comment.
Makes sense. Should we also update rotationMatrix(angle, rotationAxisVector) that systematically returns a Matrix today?
There was a problem hiding this comment.
Makes sense. Should we also update rotationMatrix(angle, rotationAxisVector) that systematically returns a Matrix today?
That is a good point, yes makes sense to use the information from rotationAxisVector when provided, and use config.matrix otherwise.
|
Thanks @rnd-debug for the latest updates. Looks solid 💪 |
|
The new function |
Related to #1160
rotationMatrixconfig.predictableto return an Array or a Matrix according to the type ofwvector.Additional request: Would it be possible to mark this PR with
hacktoberfest-acceptedtag? 😇 (more about hacktoberfest and the reason for the tag)