Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2370 +/- ##
==========================================
- Coverage 87.36% 84.85% -2.52%
==========================================
Files 49 49
Lines 7787 7763 -24
==========================================
- Hits 6803 6587 -216
- Misses 984 1176 +192
|
flying-sheep
left a comment
flying-sheep
left a comment
There was a problem hiding this comment.
looks great, but please go through all the tests you now switched from concatenate to concat and delete all the duplicate ones if any exist – I assume we test concat elsewhere and some of the changed tests now test the exact same code paths as previously existing ones.
| assert np.allclose(var_ma.compressed(), var_ma_ref.compressed()) | ||
|
|
||
|
|
||
| @mark_legacy_concatenate |
There was a problem hiding this comment.
The possibly redundant ones start here I think
There was a problem hiding this comment.
Weirdly enough, this one is simultaneously testing two things not currently tested - how does join affect obs columns and X when X is dense. There is a similar test for sparse, test_concatenate_sparse which doesn't do the obs. Should they be refactored for parametrization? Maybe, I'd need to look closer to see if that's feasible.
That's why I did refactor the other test where the big diff is - the behavior literally changed and would not have been tested otherwise.
There was a problem hiding this comment.
yeah, I just mean that there are a bunch of tests where you remove @mark_legacy_concatenate and replace concatenate with concat. Are there no pre-existing concat tests that cover what these ones do?
2 participants
backedinternal checking #2369