Thoughts on this?

It looks like we need more active core devs familiar with the dictionary learning code. Sorry. Perhaps I'll find a moment to brush up.

I agree that it seems strange for this constant to be hard-coded, but I'm also not familiar with the code. Can you explain the use-case and why this is the appropriate solution?

To give an example, if you have very noisy data, it may occur that some of the atoms are basically reconstructing the noise in the data. Instead of keeping those noise representing atoms for repeated refinement, we would better off to reinitialize them and possibly capture a true feature of the data.

Edit: Should add that was constitutes noise can vary between datasets. Hence why it would be desirable to set a different threshold here for reinitialization.

Thanks for taking a look. 😄

Ping @vene. I'd like to be able to look at these PRs, @jairkham, but my unfamiliarity with dictionary learning is making me down-prioritise them within a long list of reviewing.

Based on discussion, it sounds like this shouldn't be parameterizable. However we still would like to bump the threshold for when a basis gets tossed and reseeded from the data. The crucial point being it is better to not continue with a bad atom for too long. Please feel free to correct me if I'm missing something.

cc @GaelVaroquaux

The _update_dict method has been refactored and an atom is not resampled based on its norm anymore. Instead it's resampled based on its use, more precisely it's based on ||code[:,k]|| (where code is (n_samples, n_components)). The value is still arbitrary, but consistent with the spams implementation.

Based on the experiments in #19198, this refactoring has greatly improved the resampling of useless atoms and eventually the quality of the dictionary.

I think that we can consider that #19198 kind of fixed this. Feel free to reopen if you think otherwise.