-
Notifications
You must be signed in to change notification settings - Fork 8
test: add initial test of optimizer #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@sbillinge I am fine with running the tests locally and posting a screenshot if that makes things easier for you. This time, though, could you try running it on your end? I want to make sure everything is portable and working okay. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very nice and clean code, thanks for that! Please see some small style comments
tests/input/init_stretch.txt
Outdated
@@ -0,0 +1,2 @@ | |||
1.00118304834876 0.989988873893041 1.03013070747622 0.987599056706898 0.993727052526116 0.993574694986851 0.992490127608507 1.01538515004735 0.995493487806819 0.994741356008104 0.9969876201317 0.984590423544293 0.995875540757635 1.00121917461288 1.0008531689609 1.01414483951976 0.998022231896728 0.980312243837415 0.997840579466313 0.992141316223427 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, this is not a comment on this file but I wanted a place to put a general comment.....let's maybe have a small README in here that explains all these files and their origin. I imagine that it is a fit that works in Ran's matlab code and so it is a kind of "ground truth" for our Python conversion (as you mention a test that is kind of both not useful (slow and not a unit test" but super useful because it immediately tells us if we broke anything..... Anyway, a README that sitting in this folder that kind of lists the files and the origin story of this test and how it is supposed to be used would be good.
One way we could use such a test in the future in CI is not run it on every PR, but do run it when we merge to main. We have tests that do that for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Let me know what you think of the one I added.
Sure, I can do that. Once we have a test we may want to skpkg it so it has CI running and so on. |
Absolutely. I wasn't sure how to set that up but once you are satisfied with the test, it would be good practice. Also, to speed things up, the test now runs a small (and fixed) number of optimizations rather than a full one. It's still not quick but it should be under ten seconds now. |
@john-halloran this looks good. Did you see the request for the readme in the |
Hah, I didn't expect you to be so quick. It was on the way and I just pushed it as well. Thanks for the speedy review. |
Here is the first test for sNMF. It is fairly slow, so I have marked it as such. It is both not very useful and also extremely useful. For the time being, it is running the entire optimization on a known, reliable set of matrices, and asserting that the solution superficially looks to have at least a certain quality. Eventually, the goal will be to have tests for every individual function to track regressions. For now, this will at least make it less likely that I break things as I keep working!