fix presence matrix wrong shape #236
Conversation
| @@ -82,6 +82,8 @@ def test_base_builder_creation( | |||
| # Presence matrix should exist with the correct dimensions | |||
| for exp_name in ["homo_sapiens", "mus_musculus"]: | |||
| fdpm = census[CENSUS_DATA_NAME][exp_name].ms[MEASUREMENT_RNA_NAME][FEATURE_DATASET_PRESENCE_MATRIX_NAME] | |||
| fdpm_matrix = fdpm.read().coos().concat() | |||
| assert fdpm_matrix.shape[0] == 4 # 4 datasets | |||
There was a problem hiding this comment.
Each presence matrix should only have rows for the datasets that the experiment it is associated with. So 2 (distinct) datasets in each presence matrix for this test, 4 features each, 8 rows total. See discussion. Let me know if I'm misunderstanding.
There was a problem hiding this comment.
Yeah, this assert is wrong. Andrew has it right as I read the conftest code.
Remember, per Pablo's spec, it is the number of datasets that contribute data to the specific experiment, not the number in the total census (i.e., union of all organisms)
There was a problem hiding this comment.
As mentioned in the (edited) description, this wasn't the previous behavior which is what this PR is trying to restore. Since that is not compliant with the SOMA abstract spec, we'll probably want to close this and work towards a more permanent fix.
There was a problem hiding this comment.
Yeah, I was misunderstanding the issue at hand.
Here are some suggested constraints to assert - ie. what we would expect to be true:
- For all experiment's presence matrix, all dimension 0 coordinates (aka the dataset DataFrame's soma_joinid) should be > 0 and <= the max soma_joinid in the datasets table.
- for all experiment's presence matrix, the shape[0] == max(datasets soma_joinid) + 1
Important assumption above: the rows in the datasets DataFrame are assigned 0..n_datasets. This is just a convention (they could be anything, but we choose to sequentially assign them starting at zero).
So I would change your assertion to read:
assert fdpm_matrix.shape[0] == max(returned_datasets.soma_joinid) + 1
There was a problem hiding this comment.
BTW, to clear up the above comment - the spec is silent on the shape of the presence matrix, and only specifies its contents. I believe that fixing the shape as proposed by this PR is consistent with the spec.
| @@ -265,7 +265,7 @@ def populate_presence_matrix(self) -> None: | |||
| # sanity check | |||
| assert len(self.presence) == self.n_datasets | |||
|
|
|||
| max_dataset_joinid = max(self.presence.keys()) | |||
| max_dataset_joinid = max(d.soma_joinid for d in datasets) | |||
There was a problem hiding this comment.
this is wrong - it is going to add datasets which do not exist in the organism's experiment.
Edit: my misunderstanding of the defect being fixed. This change looks correct to me.
Reverts a change that introduced a bug that led the presence matrix to have an incorrect shape.
Per the cell-census schema specification, the presence matrix needs to have N rows where N is the number of datasets belonging to the experiment. Since this is a sparse matrix, its domain will extend to the total number of datasets (i.e.
max(d.soma_joinid for d in datasets)), therefore the shape of the presence matrix (for each experiment) should equal the total number of datasets.