feat: update validation for obs['is_primary_data'] to enforce False if uns['spatial']['is_single'] is False #865

nayib-jose-gloria · 2024-04-30T20:15:04Z

Reason for Change

cellxgene-schema CLI must update validation for obs['is_primary_data'] #834

Changes

add _validate_spatial_is_primary_data to _check_spatial_obs
refactored some unit tests to use a new pytest fixture, validator with a dataset that is spatial and is_single is False (since we were reusing that set-up in a few spots)

…n_tissue]

Co-authored-by: Nayib Gloria <[email protected]>

…f uns['spatial']['is_single'] is False

# Conflicts: # cellxgene_schema_cli/tests/fixtures/examples_validate.py # cellxgene_schema_cli/tests/test_schema_compliance.py # cellxgene_schema_cli/tests/test_validate.py

nayib-jose-gloria · 2024-04-30T21:05:35Z

cellxgene_schema_cli/cellxgene_schema/validate.py

@@ -1000,13 +1000,14 @@ def _validate_obsm(self):
                    self.errors.append(
                        f"Suffix for embedding key in 'adata.obsm' {key} does not match the regex pattern {regex_pattern}."
                    )
-            else:
+            elif key.lower() != "spatial":


diff is from #827 merged into main

nayib-jose-gloria · 2024-04-30T21:05:40Z

cellxgene_schema_cli/cellxgene_schema/validate.py

                if not re.match(regex_pattern, key):
                    self.errors.append(
                        f"Embedding key in 'adata.obsm' {key} does not match the regex pattern {regex_pattern}."
                    )
                self.warnings.append(
-                    f"Embedding key in 'adata.obsm' {key} does not start with X_ and thus will not be available in Explorer"
+                    f"Embedding key in 'adata.obsm' {key} is not 'spatial' nor does it start with 'X_'. Thus, it will "


diff is from #827 merged into main

nayib-jose-gloria · 2024-04-30T21:05:56Z

cellxgene_schema_cli/cellxgene_schema/validate.py

+            self.errors.append(
+                "'spatial' embedding is forbidden in 'adata.obsm' if " "adata.uns['spatial']['is_single'] is not set."
+            )
+


diff is from #827 merged into main

nayib-jose-gloria · 2024-04-30T21:06:20Z

cellxgene_schema_cli/tests/fixtures/examples_validate.py

@@ -142,7 +142,7 @@
            "HANCESTRO:0575",
            "HsapDv:0000003",
            "donor_1",
-            "nucleus",
+            "na",


diff is from #827 merged into main

# Conflicts: # cellxgene_schema_cli/cellxgene_schema/validate.py # cellxgene_schema_cli/tests/test_schema_compliance.py # cellxgene_schema_cli/tests/test_validate.py

nayib-jose-gloria · 2024-05-01T21:16:27Z

cellxgene_schema_cli/tests/test_schema_compliance.py

@@ -1993,30 +2010,18 @@ def test_obsm_values_no_X_embedding__non_spatial_dataset(self, validator_with_ad
            "WARNING: Validation of raw layer was not performed due to current errors, try again after fixing current errors.",
        ]

-    @pytest.mark.parametrize(


split this test up into two for simplicity in the set-up

nayib-jose-gloria · 2024-05-01T21:18:46Z

cellxgene_schema_cli/tests/test_schema_compliance.py

-        validator.validate_adata()
-        assert validator.errors == []
-
-    def test_obsm_values_spatial_embedding_present__is_single_false(self, validator_with_visium_assay):


moved this test into test_validate.py as test__validate_spatial_is_single_false_ok and renamed now that we have the fixture adata_spatial_is_single_false with spatial embedding present

MillenniumFalconMechanic · 2024-05-01T21:21:42Z

cellxgene_schema_cli/cellxgene_schema/validate.py

+        """
+        Validate is_primary_data for spatial datasets.
+        """
+        is_single = (


Could we use _is_single() here once this is rebased on main?

ah yes good call

MillenniumFalconMechanic · 2024-05-01T21:24:14Z

cellxgene_schema_cli/cellxgene_schema/validate.py

+            return
+        if is_single is False and obs["is_primary_data"].any():
+            self.errors.append(
+                "When uns['spatial']['is_single'] is False, " "obs['is_primary_data'] must be False for all rows."


There's a break in the string here (is False, " "obs['is_primary_data']).

nayib-jose-gloria

Note: I decided not to explicitly add a test_schema_compliance test case for allowing is_primary_data to be True or False if uns["spatial"]["is_single"] is None, because we implicitly cover this in test_validate.py by running test__validate_with_h5ad_valid_and_labels, which uses a fixture that has is_primary_data values True and False and no spatial in uns. I could be convinced to add the test case for clarity, but leaning towards no. LMK what you think

MillenniumFalconMechanic · 2024-05-01T22:13:21Z

cellxgene_schema_cli/tests/test_schema_compliance.py

        validator.validate_adata()
        assert validator.errors == []
        assert validator.is_spatial is True

+    def test_obsm_values_no_X_embedding__slide_seq_v2_dataset(self, validator_with_slide_seq_v2_assay):
+        validator = validator_with_slide_seq_v2_assay
+        validator.adata.uns["default_embedding"] = "spatial"


Looks like this test is missing validator.validate_adata() and the corresponding assertions.

sad, it must've gotten lost along the way during a merge resolution

MillenniumFalconMechanic · 2024-05-01T22:50:21Z

cellxgene_schema_cli/cellxgene_schema/validate.py

+        obs = getattr_anndata(self.adata, "obs")
+        if obs is None or "is_primary_data" not in obs:
+            return
+        if self._is_single() is False and obs["is_primary_data"].any():


How are "secondary errors" typically handled? For example, should this error be reported for a non-spatial dataset that has (incorrectly) specified uns.spatial.is_single = False?

since we accumulate all errors by appending to "self.errors", something like that would be handled wherever the uns check is being done. In this case, we should handle that in check_spatial_uns. I'll double check if we already are and add it in if not.

Oh wait I see what you mean--yes, we do report secondary errors like this. That way, if the problem was that they did mean for it to be spatial but accidentally used the wrong assay, we still tell them in 1 run all the other issues associated with it. Generally, we try to report every possible error in one go even if they'd all be fixed with 1 upstream correction. With some exceptions--we don't validate the X matrix, for instance, if there are issues with dependent columns before then, because its an expensive operation to run if we know there will be failures. But we do print a warning letting them know that this was the case--WARNING: "Validation of raw layer was not performed due to current errors, try again after fixing current errors."

I do think its fine that we exit early w/o checking spatial values in check_spatial_uns if assay is not 'EFO:0010961' (Visium Spatial Gene Expression) or 'EFO:0030062' (Slide-seqV2), since curators have not requested otherwise and the errors would end up being the same for each one (i.e. this key should not exist if its not visium X times)

Got it, thanks Nayib!

MillenniumFalconMechanic · 2024-05-01T22:50:59Z

Looks good, thanks Nayib! I have added a couple of comments.

MillenniumFalconMechanic · 2024-05-01T22:58:36Z

cellxgene_schema_cli/tests/test_schema_compliance.py

-        validator.adata.obsm["spatial"] = validator.adata.obsm["X_umap"]
+    def test_obsm_values_no_X_embedding__visium_dataset(self, validator_with_visium_assay):
+        validator = validator_with_visium_assay
+        validator.adata.uns["default_embedding"] = "spatial"


I am not familiar with the complete, required shape for adata here but I see there are a couple of differences between the previous version and validator_with_visium_assay (e.g. validator.adata.obsm["harmony"] is no longer defined, validator.adata.uns["default_embedding"] is now spatial). Are these updates expected/OK?

yes, its expected

for non-spatial datasets, not having an X-prefix embedding like X_umap should result in an error (see test_obsm_values_no_X_embedding__non_spatial_dataset).

However, for spatial datasets, we do not require an X-prefix embedding, just 'spatial' embedding. This test case was originally modeled after test_obsm_values_no_X_embedding__non_spatial_dataset, which used 'harmony' as its non-X-prefix embedding to trigger the error. But I realized spatial is already a non-X-prefix embedding so we didn't need to add another one to test.

MillenniumFalconMechanic

LGTM, thanks Nayib!

nayib-jose-gloria · 2024-05-02T16:28:25Z

@brian-mott ready for your review!

brian-mott · 2024-05-02T17:55:38Z

Looks good, thanks Nayib!

MillenniumFalconMechanic and others added 16 commits April 29, 2024 11:27

feat(schema 5.1.0): validate uns[spatial]

ab3c971

Linting

b6c93ad

Updated error messages

9010444

Review updates

ec8fb63

Review updates

832b297

Reverted update

faec600

feat(schema 5.1.0): validate obs[array_col], obs[array_row] and obs[i…

4c8a739

…n_tissue]

Corrected test name

476b869

Linting

b6020a8

Updated forbidden tests

74496f5

Update cellxgene_schema_cli/cellxgene_schema/validate.py

b37c52c

Co-authored-by: Nayib Gloria <[email protected]>

Updated tests

126cdb1

Review updates

3b01478

feat: update validation for obs['is_primary_data'] to enforce False i…

1fea0cd

…f uns['spatial']['is_single'] is False

Merge branch 'main' into nayib/834-primary-data

f518647

# Conflicts: # cellxgene_schema_cli/tests/fixtures/examples_validate.py # cellxgene_schema_cli/tests/test_schema_compliance.py # cellxgene_schema_cli/tests/test_validate.py

unit test refactor

6f25b11

nayib-jose-gloria requested review from MillenniumFalconMechanic, NoopDog and Bento007 April 30, 2024 20:15

nayib-jose-gloria changed the base branch from mim/829-spatial-obs to main April 30, 2024 20:15

nayib-jose-gloria changed the base branch from main to mim/829-spatial-obs April 30, 2024 20:56

nayib-jose-gloria commented Apr 30, 2024

View reviewed changes

MillenniumFalconMechanic added 5 commits April 30, 2024 14:08

feat(schema 5.1.0): validate uns[spatial]

7fe05b2

Linting

dc30afb

Updated error messages

869c29f

Review updates

60cdf04

Review updates

a2935a5

MillenniumFalconMechanic added 3 commits April 30, 2024 15:54

Linting

d202136

Corrected test

07ae9d9

Updated tests post-rebase

4d4fb2b

MillenniumFalconMechanic force-pushed the mim/829-spatial-obs branch from 83f09a7 to 4d4fb2b Compare April 30, 2024 23:10

nayib-jose-gloria added 3 commits May 1, 2024 12:22

Merge branch 'mim/829-spatial-obs' into nayib/834-primary-data

7be5f4b

# Conflicts: # cellxgene_schema_cli/cellxgene_schema/validate.py # cellxgene_schema_cli/tests/test_schema_compliance.py # cellxgene_schema_cli/tests/test_validate.py

fix test

6ccd093

lint

5a3c660

MillenniumFalconMechanic force-pushed the mim/829-spatial-obs branch from 1888316 to b933f55 Compare May 1, 2024 19:24

Base automatically changed from mim/829-spatial-obs to main May 1, 2024 20:28

nayib-jose-gloria added 2 commits May 1, 2024 17:09

Merge branch 'main' into nayib/834-primary-data

01bea67

# Conflicts: # cellxgene_schema_cli/cellxgene_schema/validate.py # cellxgene_schema_cli/tests/test_schema_compliance.py # cellxgene_schema_cli/tests/test_validate.py

tests

eb31ce3

nayib-jose-gloria commented May 1, 2024

View reviewed changes

MillenniumFalconMechanic reviewed May 1, 2024

View reviewed changes

refactor + linting

580f8e0

nayib-jose-gloria commented May 1, 2024

View reviewed changes

MillenniumFalconMechanic reviewed May 1, 2024

View reviewed changes

fix test

8bc61a9

nayib-jose-gloria requested a review from MillenniumFalconMechanic May 2, 2024 14:52

nayib-jose-gloria and others added 2 commits May 2, 2024 11:22

Merge branch 'main' into nayib/834-primary-data

db7801d

lint

680e62f

MillenniumFalconMechanic approved these changes May 2, 2024

View reviewed changes

nayib-jose-gloria merged commit d8e94b5 into main May 2, 2024
6 checks passed

nayib-jose-gloria deleted the nayib/834-primary-data branch May 2, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update validation for obs['is_primary_data'] to enforce False if uns['spatial']['is_single'] is False #865

feat: update validation for obs['is_primary_data'] to enforce False if uns['spatial']['is_single'] is False #865

nayib-jose-gloria commented Apr 30, 2024 •

edited

Loading

nayib-jose-gloria Apr 30, 2024

nayib-jose-gloria Apr 30, 2024

nayib-jose-gloria Apr 30, 2024

nayib-jose-gloria Apr 30, 2024

nayib-jose-gloria May 1, 2024

nayib-jose-gloria May 1, 2024

MillenniumFalconMechanic May 1, 2024

nayib-jose-gloria May 1, 2024

MillenniumFalconMechanic May 1, 2024

nayib-jose-gloria left a comment

MillenniumFalconMechanic May 1, 2024

nayib-jose-gloria May 2, 2024

MillenniumFalconMechanic May 1, 2024

nayib-jose-gloria May 2, 2024

nayib-jose-gloria May 2, 2024

nayib-jose-gloria May 2, 2024

MillenniumFalconMechanic May 2, 2024

MillenniumFalconMechanic commented May 1, 2024

MillenniumFalconMechanic May 1, 2024

nayib-jose-gloria May 2, 2024

MillenniumFalconMechanic left a comment

nayib-jose-gloria commented May 2, 2024

brian-mott commented May 2, 2024

feat: update validation for obs['is_primary_data'] to enforce False if uns['spatial']['is_single'] is False #865

feat: update validation for obs['is_primary_data'] to enforce False if uns['spatial']['is_single'] is False #865

Conversation

nayib-jose-gloria commented Apr 30, 2024 • edited Loading

Reason for Change

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayib-jose-gloria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MillenniumFalconMechanic commented May 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MillenniumFalconMechanic left a comment

Choose a reason for hiding this comment

nayib-jose-gloria commented May 2, 2024

brian-mott commented May 2, 2024

nayib-jose-gloria commented Apr 30, 2024 •

edited

Loading