-
Notifications
You must be signed in to change notification settings - Fork 1
Sparse matrix rebase #52
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
base: main
Are you sure you want to change the base?
Conversation
|
OK first we made the CI tests actually run all the way through - since they are split out into different steps, if one fails then the others formerly wouldn't run, so we wouldn't have a complete picture of all our test failures so we could actually act on them. Adjusted the workflow to run each of those (which should probably just be one step but whatevs). So then the first run here: https://github.com/Aharoni-Lab/indeca/actions/runs/21265407119/job/61203238082
so that's a lot of failing tests, so much so that the github interface can't really load the whole stdout. the problem for pretty much all of those is totally trivial: the pydantic model expects a Now in the latest run:
so 265 failing tests down to 8 with a single line change, not bad. The remaining 8 tests seem like "actual" failures, in the sense that they're failing for a nontrivial reason. Normally what we would want to do is a) be running and looking at the tests as we commit code so that we resolve bugs either before they are commited or immediately after, or b) if there were bugs introduced in some series of commits, do a however the bug was introduced in 58fc9a5 which is the first commit in this branch (which corresponds to #35 ), and that commit is enormous. so to bisect we would have to again rebase this branch, cherry picking the line change into that commit, and do a bunch of other fun git things to retroactively rewrite history. so now the situation is that
in the future to avoid this we should
i think one of the problems here is that the tests take eons to run locally, and that's actually a real problem worth resolving: fast tests are tests that are run frequently, and tests that are run frequently are tests that help us write good code. when i'm working on a package i expect to just be constantly running the tests to check my work, and so having the test take so long actually materially contributes to the package being difficult to work on. |
This is #40 rebased against the reset
mainbranch for a clean diff. I didn't want to just force push against @daharoni 's branch since it's not mine.the prior merge of #34 handled a large amount of the removals which made the prior diff so large, so this branch now includes all the other open branches and is caught up to main:
see the
sparse-matrix-compression...branch in cyan, and then howadd-profiler-and-vis,refactor-pipeline,refactor-deconfanddataclass-refactorare all just references to different commits also contained withinsparse-matrix...we could do each of those one by one, rebasing each time, but because each of them is built on top of the others, rather than each being independently branched from main, it would be much more work to actually review and make changes to each of them: if we did code review and made changes to
refactor-deconv, then we would need to rebaserefactor-pipeline,add-profiler-and-vis, andsparse-matrix...on top of those changes.so the goal now is to get the repo caught up and in such a state where we can make incremental improvements to it as independent PRs that are reviewed and merged without getting too far away from main.