Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix presence matrix wrong shape #236

Merged
merged 2 commits into from
Mar 2, 2023
Merged

fix presence matrix wrong shape #236

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c73db6
fix presence matrix wrong shape
ebezzi Mar 1, 2023
9aa935e
Improved assertions
ebezzi Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tools/cell_census_builder/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ def build_step4_populate_X_layers(
populate_X_layers(assets_path, filtered_datasets, experiment_builders, args)

for eb in reopen_experiment_builders(experiment_builders):
eb.populate_presence_matrix()
eb.populate_presence_matrix(filtered_datasets)

logging.info("Build step 4 - Populate X layers - finished")

Expand Down
4 changes: 2 additions & 2 deletions tools/cell_census_builder/experiment_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ def create_X_with_layers(self) -> None:
platform_config=CENSUS_X_LAYERS_PLATFORM_CONFIG[layer_name],
)

def populate_presence_matrix(self) -> None:
def populate_presence_matrix(self, datasets: List[Dataset]) -> None:
"""
Save presence matrix per Experiment
"""
Expand All @@ -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)
Copy link
Contributor

@bkmartinjr bkmartinjr Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# LIL is fast way to create spmatrix
pm = sparse.lil_array((max_dataset_joinid + 1, self.n_var), dtype=bool)
Expand Down
2 changes: 2 additions & 0 deletions tools/cell_census_builder/tests/test_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was misunderstanding the issue at hand.

Here are some suggested constraints to assert - ie. what we would expect to be true:

  1. 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.
  2. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fdpm_df = fdpm.read().tables().concat().to_pandas()
n_datasets = fdpm_df["soma_dim_0"].nunique()
n_features = fdpm_df["soma_dim_1"].nunique()
Expand Down