[mlpack-git] [mlpack] Warn if ratings of 0 are found (#376)

[Stephen Tu]

Ignoring the zeros is pretty baked into the CF/AMF code right now. Here are a few instances I found when I was looking at the code:

https://github.com/mlpack/mlpack/blob/660fd96eecf88245555a8651e1cbeff50cd66686/src/mlpack/methods/cf/cf_impl.hpp#L170
https://github.com/mlpack/mlpack/blob/660fd96eecf88245555a8651e1cbeff50cd66686/src/mlpack/methods/amf/init_rules/average_init.hpp#L42
https://github.com/mlpack/mlpack/blob/660fd96eecf88245555a8651e1cbeff50cd66686/src/mlpack/methods/amf/update_rules/svd_batch_learning.hpp#L91
https://github.com/mlpack/mlpack/blob/660fd96eecf88245555a8651e1cbeff50cd66686/src/mlpack/methods/amf/update_rules/svd_complete_incremental_learning.hpp#L84

Hence the pull request. I did see the `sp_mat` constructor taking a `check_for_zeros` parameter. However, the comment in the armadillo documentation states (http://arma.sourceforge.net/docs.html#SpMat)

    If check_for_zeros is set to false, the values vector is assumed to contain no zero values

As an API user, when I read something like this, it suggests to me that putting explicit zero indices in is incorrect. Whether or not it actually is, I assume you are more familiar with since I'm guessing you wrote some of the sparse matrix code? The point is, to set `check_for_zeros = false`, and then explicitly stuff zero values in, seems to break the assumption of the parameter. 

I'm not that opposed to the idea, just wanted to point out some discrepancies before proceeding. 

---
Reply to this email directly or view it on GitHub:
https://github.com/mlpack/mlpack/pull/376#issuecomment-69620397
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20150112/16da73f5/attachment.html>