scverse / scverse/spatialdata

`TableModel().validate()` intentionally skips check for unique `instance_key` values (grouped by `region_key` values)

Open
#715 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

element: table 📑 models needs: triage priority: low
Dominant language
Python
Stars
394
Forks
95
Avg merge
4d 3h
Merged PRs (30d)
7

Description

The check of unique instance_key values (after grouping them by region_key values) is an expensive check, therefore it is performed only during table parsing and not validation.

Anyway, we should add a flag to validate() to allowing performing this check, and do this when writing.

We need to modify the tests for checking for this (something like uncommenting the code here below).

    @pytest.mark.parametrize("model", [TableModel])
    @pytest.mark.parametrize("region", [["sample_1"] * 5 + ["sample_2"] * 5])
    def test_table_instance_key_values_not_unique(self, model: TableModel, region: str | np.ndarray):
        region_key = "region"
        obs = pd.DataFrame(RNG.integers(0, 100, size=(10, 3)), columns=["A", "B", "C"])
        obs[region_key] = region
        obs["A"] = [1] * 5 + list(range(5))
        adata = AnnData(RNG.normal(size=(10, 2)), obs=obs)

        # check parse fails
        with pytest.raises(ValueError, match=re.escape("Instance key column for region(s) `sample_1`")):
            model.parse(adata, region=region, region_key=region_key, instance_key="A")
        # # check also validate fails
        # adata.uns[TableModel.ATTRS_KEY] = {
        #     TableModel.REGION_KEY: region,
        #     TableModel.REGION_KEY_KEY: region_key,
        #     TableModel.INSTANCE_KEY: "A",
        # }
        # with pytest.raises(ValueError, match=re.escape("Instance key column for region(s) `sample_1`")):
        #     model().validate(adata)
        # del adata.uns[TableModel.ATTRS_KEY]

        adata.obs["A"] = [1] * 10

        # check parse fails
        with pytest.raises(ValueError, match=re.escape("Instance key column for region(s) `sample_1, sample_2`")):
            model.parse(adata, region=region, region_key=region_key, instance_key="A")
        # # check also validate fails
        # adata.uns[TableModel.ATTRS_KEY] = {
        #     TableModel.REGION_KEY: region,
        #     TableModel.REGION_KEY_KEY: region_key,
        #     TableModel.INSTANCE_KEY: "A",
        # }
        # with pytest.raises(ValueError, match=re.escape("Instance key column for region(s) `sample_1, sample_2`")):
        #     model().validate(adata)
        # del adata.uns[TableModel.ATTRS_KEY]

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating TableModel.validate(), TableModel.parse(), and the test named test_table_instance_key_values_not_unique. Review how parsing performs the grouped instance_key uniqueness check and how writing invokes validation. Add coverage for the optional validation check, including both duplicate-region cases, and confirm writing performs the check.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
data
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.