Initial commit of John refactor of SNMF #140
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## john-development #140 +/- ##
=================================================
Coverage 91.96% 91.96%
=================================================
Files 7 7
Lines 112 112
=================================================
Hits 103 103
Misses 9 9 🚀 New features to boost your workflow:
|
|
@john-halloran do you want me to merge this with the ambiguous variable issue so we can move forward, or did you want to push a fix and have me merge it? I know you were not sure what the variable did, but having some fix that captures that allows this to be merged. One approach is to not change it but capture the issue in an "issue" that we can return to later. Another is to change the variable to something that passes pre-commit but captures the uncertainty so that we can, in principle, return to it later if we want (a Just let me know so we can get this merged and start getting your work reviewed. |
|
No, don't merge yet. I have resolved the pre-commit issues and a bunch of others besides. But there's a couple more major known issues - things that essentially prevent the program from working - that I want to resolve this week. Then I'll update the PR and we can start merging/reviewing. |
I trust you on this. As a general matter, we tend to like slightly more frequent pushes of intermediate work so that you can get earlier feedback. If you don't want things merged, set the PR to be "draft" in GH and then when it is ready for final review and merge, you can undraft it and ping me. But I often look over intermediate work at a low level and can give feedback, as can others who are monitoring the repo. |
|
Okay, understood. Based on that, I have updated the PR with the pre-commit fixes and also all the other work I've done. There's still a lot I'm working on, but it's far enough along to share more. |
There was a problem hiding this comment.
please let me know what you want me to do with this. Do you want me to merge it? I can try and give feedback too, but I don't have much yet as you are basically just writing code to clone the matlab, right? With this in mind, I would be happy to merge it in whatever condition it is in to help move things forward. We can make issues and do cleaning after that.
|
@sbillinge I think this is ready to merge now. I was working with Filippo last week on his data, and while there are still issues, the code now is capable of the same output as the MATLAB. |
That's really amazing news! Do you have suggestions about any kind of refactoring to make it more modular or give it a different interface? |
|
@sbillinge Yes, there is certainly plenty of refactoring to come. I'm still planning to model it on the scikit-learn base NMF class, although fully getting it compatible with that may be a longer term effort. In the meantime I am implementing currently the custom normalization used in the original MATLAB. This isn't strictly part of the core logic, but is necessary to get results that actually look like XRD/PDF components and can be plotted normally. |
No description provided.