Compute noise_variance_ in PCA implementation
#6234
Conversation
jcrist
force-pushed
the
pca-noise-variance
branch
from
f73ec37 to
c4939b6
Compare
| @@ -128,9 +128,6 @@ void fit_impl(raft::handle_t& handle, | |||
| streams, | |||
| n_streams, | |||
| verbose); | |||
| for (std::uint32_t i = 0; i < n_streams; i++) { | |||
| handle.sync_stream(streams[i]); | |||
| } | |||
There was a problem hiding this comment.
This sync point was unneeded, it was already handled after exiting this branch below.
| true, | ||
| stream); | ||
| } else { | ||
| raft::matrix::setValue(noise_vars, noise_vars, math_t{0}, 1, stream); |
There was a problem hiding this comment.
noise_vars is a len-1 device array that I want to set to 0. This seems to work (and is used at least one other place in cuml already), but I'm honestly not sure if it's the best spelling of that. In particular, I don't understand why a method for filling an output array with a single value also takes in an input array (which is the same as the output array in all calling locations I can find).
There was a problem hiding this comment.
It would be better to use raft::matrix::fill from raft/include/matrix/init.cuh Line 68. It is the more up-to-date function. You would need to create a mdspan to use it as such:
auto noise_vars_span = raft::make_device_vector_view(noise_vars, 1);
Previously `noise_vars` was an output parameter passed to the cuda PCA implementation, but it was unimplemented. This adds support for computing `noise_vars` in the cuda code, and tests that the results are valid by comparing to the scikit-learn implementation. The previous code would always have a `noise_variance_` of 0, resulting in downstream issues interpreting results after converting a cuml estimator to its sklearn equivalent (e.g. broken `score_samples`).
jcrist
force-pushed
the
pca-noise-variance
branch
from
c4939b6 to
4a927b3
Compare
| true, | ||
| stream); | ||
| } else { | ||
| raft::matrix::setValue(noise_vars, noise_vars, math_t{0}, 1, stream); |
There was a problem hiding this comment.
It would be better to use raft::matrix::fill from raft/include/matrix/init.cuh Line 68. It is the more up-to-date function. You would need to create a mdspan to use it as such:
auto noise_vars_span = raft::make_device_vector_view(noise_vars, 1);
| // Compute the scalar noise_vars defined as (pseudocode) | ||
| // (n_components < min(n_cols, n_rows)) ? explained_var_all[n_components:].mean() : 0 | ||
| if (prms.n_components < prms.n_cols && prms.n_components < prms.n_rows) { | ||
| raft::stats::mean(noise_vars, |
There was a problem hiding this comment.
Can you use the newer mdspan-API of this mean function as well?
There was a problem hiding this comment.
It's my understanding that since the PCA code here doesn't create streams from the handle (#2470), and is mostly using the legacy APIs here that take streams instead of handles, that switching to the mdspan APIs instead would require a substantial refactor. I'm happy to take that on in a follow-up, but would prefer to port all the code to the newer APIs at the same time, rather than try to force it in as part of this PR.
There was a problem hiding this comment.
Yes that's a valid point, let's keep that port for an other dedicated PR.
| pytest.param(20, 10, id="n_features <= n_components"), | ||
| ], | ||
| ) | ||
| def test_noise_variance_zero(n_samples, n_features): |
There was a problem hiding this comment.
Can a test other than zero be added? To check the correctness of the computation
There was a problem hiding this comment.
This test is checking the case where the noise variance is defined as zero since there weren't enough samples or features. Other cases where it's non zero are already tested thoroughly above on line 75.
| // Compute the scalar noise_vars defined as (pseudocode) | ||
| // (n_components < min(n_cols, n_rows)) ? explained_var_all[n_components:].mean() : 0 | ||
| if (prms.n_components < prms.n_cols && prms.n_components < prms.n_rows) { | ||
| raft::stats::mean(noise_vars, |
There was a problem hiding this comment.
Yes that's a valid point, let's keep that port for an other dedicated PR.
|
/merge |
Previously
noise_varswas an output parameter passed to the cuda PCA implementation, but it was unimplemented. This adds support for computingnoise_varsin the cuda code, and tests that the results are valid by comparing to the scikit-learn implementation.The previous code would always have a
noise_variance_of 0, resulting in downstream issues interpreting results after converting a cuml estimator to its sklearn equivalent (e.g. brokenscore_samples).