Loop cleanup and scikit-learn style parameters

[john-halloran]

No description provided.

[github-actions]

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.
For best practices, please visit
https://billingegroup.github.io/scikit-package/frequently-asked-questions.html#billinge-group-standards.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (john-development@156511e). Learn more about missing BASE report.

Additional details and impacted files

@@                 Coverage Diff                 @@
##             john-development     #146   +/-   ##
===================================================
  Coverage                    ?   91.96%           
===================================================
  Files                       ?        7           
  Lines                       ?      112           
  Branches                    ?        0           
===================================================
  Hits                        ?      103           
  Misses                      ?        9           
  Partials                    ?        0           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sbillinge]

@john-halloran I am not paying close attention to this. Could you maybe share what you want me to do? Give feedback? Merge ? Some words about the intent of the PR would be helpful. The title is good and makes sense, but is a bit vague as this seems to be the goal of a much larger refactor than will happen only a single PR

[john-halloran]

Yes, of course. The goal of this PR is to move the sNMF implementation away from the original, specific style into one that is more general and better aligned with scikit-learn. It is not a full alignment with scikit-learn, of course, but it does do the following:

• We no longer use any dependencies other than those scikit-learn uses (all optimization moved to scipy)
• More steps are handled off-the-shelf instead of explicitly coded
• We no longer iterate in a bespoke order (Ran confirmed this was only done for minor, performance reasons)
• "tol" is a user parameter just like in scikit-learn, as is "max_iter" and "components"
  We can discuss more when we meet soon, but my hope was that you'd just merge for now unless you have critical feedback. This is the build I'm planning to use for my final project as well.

[sbillinge]

so a general comment is that this looks like at least four PRs are needed, not just one. As we move away from your massive (and beautifully executed) code translation job to more functional code, we want to move to very small granular PRs that only do one thing at a time.

[john-halloran]

Understood. With that in mind for the future, do you think it's okay to merge this one, this once? If not, I'd be happy to go back and granulate the changes.