Number of singular values argument ignored by eqcorrscan.utils.mag_calc.svd_moments()

[nackerley]

The only place the n_svs argument is used by eqcorrscan.utils.mag_calc.svd_moments(u, s, v, stachans, event_list, n_svs) is to select a subset of the singular values s_working: https://github.com/eqcorrscan/EQcorrscan/blob/3aac00b4c9dab0416efadbb64d341cc948d3f36b/eqcorrscan/utils/mag_calc.py#L935-L942 After this, s_working is never used; only U_working is, and I believe it is being accessed in such a way as to select only the first singular value.

More broadly, I've been struggling with getting eqcorrscan.utils.clustering.svd(stream_list) and eqcorrscan.utils.mag_calc.svd_moments() to work properly together. I'm working on a PEP8 compliant pull request which fixes this issue, and makes the code more readable, lint-free, and makes it easier to manage the pernicious event_list input argument.

Please let me know if there are peculiarities I should be aware of, particularly with respect to designing regression tests. From a cursory inspection, it seems that currently the tests only check the dimensionality of the returned arguments, not the values. See below: https://github.com/eqcorrscan/EQcorrscan/blob/3aac00b4c9dab0416efadbb64d341cc948d3f36b/eqcorrscan/tests/mag_calc_test.py#L189-L214 https://github.com/eqcorrscan/EQcorrscan/blob/3aac00b4c9dab0416efadbb64d341cc948d3f36b/eqcorrscan/tests/clustering_test.py#L224-L246

[calum-chamberlain]

Thanks for these notes - you are right on all counts. I'm not sure what is going on with the n_SVs and s_working. It would be great to see this cleaned up and more usable. From memory, with svd_mag_calc, one reason we don't test for accuracy is because, due to the random pairing, we get different absolute answers every-time (but the relative moments should be consistent).

[calum-chamberlain]

It looks like this has gone quite stale - at some point I might get around to it myself, but it is not high on my priority list. If anyone wants to contribute to fix this that would be great.