Implementation of Standard scaler#143
Conversation
c4153c4 to
f866035
Compare
Mec-iS
left a comment
Mec-iS
left a comment
There was a problem hiding this comment.
Thanks very much for your contribution!
These are the first suggetions that came to my mind.
| } | ||
|
|
||
| /// Extract a single column from a matrix and return it as a matrix. | ||
| fn take_column_from_matrix<T, M>(matrix: &M, column_index: usize) -> M |
There was a problem hiding this comment.
These two function look really useful, maybe you should consider adding them to linalg::BaseMatrix implementation as they are more general than the scaler implementation.
It would be nice to have something like DenseMatrix::take_column(...) (I thought we already have something like it?)
There was a problem hiding this comment.
there is already a take implementation for matrices.
There was a problem hiding this comment.
yes exactly, I also use the Matrix::take interface here, but I want to specialize it for taking columns (then you need to paramterize it via axis, e.g. Matrix::take(&[column_index], 1)). This is helpful because then I don't need to repeat this in my code and can test it in a central place (I always like to mess up the axis indices 😆 ). So if it is ok, I would still like to keep build_matrix_from_columns (or put it in the Matrix-implementation).
Apart from this I would also be happy to put any of the two methods (build_matrix_from_columns | take_column) into the Matrix imlementation. Should I do it? For which of the two methods?
There was a problem hiding this comment.
If they have generic functionalities and they can be reused, not strictly tied to the implementation of the module, they can be moved to modules with a more generic functionalities so that other modules can make use of them. I think you can move take_columns_from_matrix into linalg::BaseMatrix implementation so it can be used like BaseMatrix::take_columns_from_matrix; same for build_matrix_from_columns.
There was a problem hiding this comment.
As we discussed below, I think take_column_from_matrix , or rather Matrix:take_column (what you proposed earlier) I would put it into a more central place as it can be used in other places as well.
|
|
||
| /// From a collection of matrices, that contain columns, construct | ||
| /// a matrix by stacking the columns horizontally. | ||
| fn build_matrix_from_columns<T, M>(columns: Vec<M>) -> Option<M> |
There was a problem hiding this comment.
Why this is a Vec<M> instead of a Vec<Vec<T>>>
There was a problem hiding this comment.
The build_matrix_from_columns function was build by me as a helper / utility function in this module. During fit of the StandardScaler I first take individual columns from the matrix that contains the raw data then apply the necessary scaling. In short I need Vec<M> instead of Vec<Vec<T>> because take_column_from_matrix returns a Matrix. Also this is helpful because I can use the neat methods sub_scalar as well as div_scaler to apply the scaling.
There was a problem hiding this comment.
ok but for it to be general it is better to use Vec<Vec<T>> as you won't need build da matrix from a matrix but a matrix from a 2-D vector. So if you plan to make build_matrix_from_columns generic it would be better for it to accept Vecs. If this is not possible, then probably the method is not fully reusable to other modules.
There was a problem hiding this comment.
Ok that make sense. Then I would prefer not to put build_matrix_from_columns into Matrix because I would need to refactor it and it would not be optimal for this module anymore.
f866035 to
2dd74cb
Compare
|
There is an error with clippy, try to run |
I created the method `Matrix::take_column` that uses the `Matrix::take`-interface to extract a single column from a matrix. I need that feature in the implementation of `StandardScaler`.
2dd74cb to
392191d
Compare
Should be fixed now! |
Codecov Report
@@ Coverage Diff @@
## development #143 +/- ##
===============================================
+ Coverage 83.65% 83.94% +0.28%
===============================================
Files 80 81 +1
Lines 8591 8724 +133
===============================================
+ Hits 7187 7323 +136
+ Misses 1404 1401 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Nice one @titoeb, hope we can collaborate in another PR soon. |
* docs: Fix typo in doc for categorical transformer. * feat: Add option to take a column from Matrix. I created the method `Matrix::take_column` that uses the `Matrix::take`-interface to extract a single column from a matrix. I need that feature in the implementation of `StandardScaler`. * feat: Add `StandardScaler`. Authored-by: titoeb <timtoebrock@googlemail.com>
3 participants
In this branch I added an implementation for a
StandardScaleras described in issue #54.