Update uns to precomp stats to take a list as parameter #21
Update uns to precomp stats to take a list as parameter #21inkarkapen wants to merge 4 commits intorc/v1.5.0from
Conversation
danielsf
left a comment
danielsf
left a comment
There was a problem hiding this comment.
Most of this looks good. I'm confused though: doesn't your use case also require an update to precomputed_stats_to_uns so that the cell_type_mapper code can save the precomputed stats file to the nested key structure in your scrattch .h5ad files?
Also: some unit tests in
tests/diff_exp/test_precompute.py
are now failing. There are calls to uns_to_precomputed_stats that need to be updated. The offending test is
test_serialization_of_actual_precomputed_stats
(sorry I forgot to call that out to you earlier). This is where the code actually tests that a precomputed stats file that is serialized to uns is unchanged from its source data. In addition to updated the function calls in this test, I think it would be a good idea to change the test logic itself so that it tries to save the precomputed_stats data to a nested key structure in uns, rather than the current flat key structure. This will involve the change to precomputed_stats_to_uns I implicitly requested above (unless you do not need precomputed_stats_to_uns changed....?)
| tmp_dir=tmp_dir_fixture) | ||
| else: | ||
| a_data = anndata.read_h5ad(h5ad_path) | ||
| a_data.uns[uns_key] = {uns_key_nested: a_data.uns[uns_key]} |
There was a problem hiding this comment.
Just being paranoid:
The way this code is constructed, the file at h5ad_path still has the expected data at [uns_key] which means that, even if uns_to_precomputed_stats were not modified, the test would still pass, I think.
Can you write a new anndata object at a different location that only has the expected data at your nested location. I am imagining something like this:
new_h5ad_path = mkstemp_clean(dir=tmp_dir_fixture, suffix='.h5ad')
src = anndata.read_h5ad(h5ad_path)
new_data = anndata.AnnData(
obs=src.obs,
var=src.var,
X=src.X[()],
uns = {uns_key_nested: {uns_key: {src.uns[uns_key]}}}
)
new_data.write_h5ad(new_h5ad_path)
h5ad_path = new_h5ad_path
| def uns_to_precomputed_stats( | ||
| h5ad_path, | ||
| uns_key, | ||
| uns_keys_list, |
There was a problem hiding this comment.
Please update the docstring to reflect the new argument.
| return result | ||
|
|
||
|
|
||
| def _read_cluster_to_row(cluster_row_lookup): |
There was a problem hiding this comment.
Please add a unit test for this function. I know it's trivial, I just want to make sure that this is actually solving the problem we want it to solve.
|
Just pinging to ask you what you think the state of this PR is/should be. Is it no longer needed? Shall I just close without merging? |
|
I have redirected this PR to We can worry about rebasing the PR onto this branch once it is ready to merge. |
danielsf
force-pushed
the
rc/v1.4.0
branch
3 times, most recently
from
e32d6d5 to
5ead218
Compare
2 participants
No description provided.