Conversation
AtheMathmo
left a comment
AtheMathmo
left a comment
There was a problem hiding this comment.
Only had time for a brief look but everything seems fine. Thank you for getting this done!
Will review properly in the next couple days. Thanks for your patience.
| //! | ||
| //! The `Normalizer` transformer is used to transform input data | ||
| //! so that the l2 norm of each rows are all equal to 1. | ||
| //! If input data has a row with all 0, `Normalizer`` keeps the row as it is. |
|
|
||
| impl<T: Float> Transformer<Matrix<T>> for Normalizer { | ||
| fn transform(&mut self, mut inputs: Matrix<T>) -> Result<Matrix<T>, Error> { | ||
| let dists: Vec<T> = inputs.iter_rows().map(|x| dist(x)).collect(); |
There was a problem hiding this comment.
I believe we can do this more efficiently than the dist function. Vectorizing the dot product takes some work, which is why we have the utils::dot function in rulinalg.
Once we merge rulinalg 0.4.x we can update to using the MatrixNorm trait directly here.
Oh, and just in case I am wrong and the dist function is better. This line can be written as:
let dists: Vec<T> = inputs.iter_rows().map(dist).collect();
|
Updated to meet with #166. |
AtheMathmo
left a comment
AtheMathmo
left a comment
There was a problem hiding this comment.
One additional minor point, and my previous performance comment still stands.
Once we have rulinalg-0.4 we should be generic over the MatrixNorm trait so that any norm can be used.
With all that said, if you don't have time to work on this I am happy to merge it as-is and make a note of the future work in a new issue.
| pub struct Normalizer; | ||
|
|
||
| /// Create a `Normalizer`. | ||
| impl Default for Normalizer { |
There was a problem hiding this comment.
This can be derived on the above struct (though I don't think even this is necessary).
let a = Normalizer;
let b = Normalizer::default();
let c = Normalizer::new();These all do the same thing here. I'd suggest we only keep the first.
* Updating to rulinalg v0.4.2 * Slight tidy up for dbscan * Improving GMM cov decomposition error msg * Updating benchmarks with rulinalg0.4
…into sinhrks-normalizer
|
Closing for #175 . |
2 participants
Add normalize transformer (#163). Because normalizer has no parameters to fit, it is separate from #166.