Perf matrix #6
Perf matrix #6
Conversation
src/index.js
Outdated
| export default { | ||
| RandomForestClassifier, | ||
| RandomForestRegression | ||
| }; |
There was a problem hiding this comment.
I had some problems using rollup because it only exports the first one, this one works for me
There was a problem hiding this comment.
Remove the default. We don't want to export an object. Just two separate things
|
it's ok now @targos? |
src/RandomForestBase.js
Outdated
| for (i = 0; i < predictionValues.length; ++i) { | ||
| predictions[i] = this.selection(predictionValues[i]); | ||
| predictionValues = new Matrix(predictionValues).transposeView(); | ||
| var predictions = new Array(predictionValues.rows); |
There was a problem hiding this comment.
I think that this is what you need
src/utils.js
Outdated
| for (var i = 0; i < indexes.length; ++i) { | ||
| toRet.setColumn(i, X.getColumn(indexes[i])); | ||
| toRet[i] = X.getColumn(indexes[i]); |
There was a problem hiding this comment.
I prefer the setters and getters, they work with views and the performance is the same
package.json
Outdated
| @@ -47,7 +49,7 @@ | |||
| "dependencies": { | |||
| "ml-array-mean": "^1.0.0", | |||
| "ml-array-median": "^1.0.0", | |||
| "ml-cart": "1.0.1", | |||
| "ml-cart": "^1.0.7", | |||
| "ml-matrix": "^3.0.0", | |||
There was a problem hiding this comment.
this should be the version 5
|
ok, I made those changes @maasencioh, it's ok for you? |
src/RandomForestBase.js
Outdated
| var predictions = new Array(predictionValues.length); | ||
| for (i = 0; i < predictionValues.length; ++i) { | ||
| predictions[i] = this.selection(predictionValues[i]); | ||
| predictionValues = new Matrix(predictionValues).transposeView(); |
There was a problem hiding this comment.
maybe the use of a WrapperMatrix1D would be faster
There was a problem hiding this comment.
the only "problem" is that this will force you to use the setters and getters
There was a problem hiding this comment.
I think that I cannot use that class because predictionValues is a 2D matrix, the other one is to use WrapMatrix2D but is the same as Matrix, I have to use tranposeView
There was a problem hiding this comment.
transposeView works on WrapMatrix2D (that was a typo indeed)
There was a problem hiding this comment.
ok, I made this change
src/RandomForestBase.js
Outdated
| predictionValues = new Matrix(predictionValues).transposeView(); | ||
| var predictions = new Array(predictionValues.rows); | ||
| for (i = 0; i < predictionValues.rows; ++i) { | ||
| predictions[i] = this.selection(predictionValues.getRow(i)); |
There was a problem hiding this comment.
this is related with mljs/matrix#65, because is safer to use setters and getters ml-matrix
There was a problem hiding this comment.
I don't understand this one, lol.
There was a problem hiding this comment.
predictions[i] should be replaced by predictions.setRow but I think that currently is not possible
There was a problem hiding this comment.
yes but predictions is currently an array, do you think that I should use WrapMatrix1D or something, I think that's adding more complexity, no?
There was a problem hiding this comment.
indeed, I was confused, I'm sorry 😝
src/index.js
Outdated
| import {default as RandomForestClassifier} from './RandomForestClassifier'; | ||
| import {default as RandomForestRegression} from './RandomForestRegression'; | ||
|
|
||
| export { |
There was a problem hiding this comment.
you should always avoid this, use this as inspiration of how to fix it
mljs/decision-tree-cart@4ceff09
There was a problem hiding this comment.
mmmm ok, I think that I should avoid that default because that cause problems with rollup, thank you
|
so, this is ready, right? @maasencioh @targos |
@targos @maasencioh for #4