chore: groupby warn and add tests for warnings#195
Conversation
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
==========================================
- Coverage 93.48% 91.69% -1.80%
==========================================
Files 14 14
Lines 1121 1132 +11
==========================================
- Hits 1048 1038 -10
- Misses 73 94 +21
🚀 New features to boost your workflow:
|
|
@ilan-gold is there anything else you wanted? for example we still fail when outer introduces a column if it's specified in groupby. But warn when it's not |
There was a problem hiding this comment.
- I think these zarr warnings are silenced globally, no?
tool.pytest.ini_options.filterwarnings? - Could you write up what behavior your are encoding here? It seems like the point is to warn on mismatched
obscolumns.
More importantly, can we add a test for what happens to the actual on-disk data when you add either
a. A new categorical column to an existing store - do all the stores get properly updated to have the new column with the right category?
b. A categorical column with the same name but different categories to an existing stores - again, are all of the newly shuffled datasets' altered column of the same dtype i.e., do they have the same codes (which is really what matters)?
| for key in curr_keys: | ||
| if not (elem_name in {"var", "obs"} and key == "_index"): | ||
| key_count[key] += 1 |
There was a problem hiding this comment.
I think we can just get away with adding obs and var to this check. Why not? It would would go beyond categoricals, but I'm not sure that is so bad. I am not sure why obs and var are excluded. This check is relatively naive as well so we could also add some behavior around e.g., mismatched dtypes. But that appears out-of-scope for this PR, aside from maybe just adding the special warning about categoricals.
| with warnings.catch_warnings(record=True) as caught_warnings: | ||
| warnings.simplefilter("always") |
There was a problem hiding this comment.
Is this just blanket ignoring warnings? Seems bad
2 participants
fixes #193