feat: add biosample index

[Tobi1kenobi]

✨ Context

🛠 What does this PR implement

🙈 Missing

🚦 Before submitting

• Do these changes cover one single feature (one change at a time)?
• Did you read the contributor guideline?
• Did you make sure to update the documentation with your changes?
• Did you make sure there is no commented out code in this PR?
• Did you follow conventional commits standards in PR title and commit messages?
• Did you make sure the branch is up-to-date with the dev branch?
• Did you write any new necessary tests?
• Did you make sure the changes pass local tests (make test)?
• Did you make sure the changes pass pre-commit rules (e.g poetry run pre-commit run --all-files)?

[Tobi1kenobi]

Few notes:

• When joining onto eQTL catalogue three biosampleIds have no match in the biosample index: BTO_0000930 (neuroblast), EFO_0005292 (lymphoblastoid cell line), EFO_0004905 (induced pluripotent stem cell). When thinking about the other use cases for this index some thought should go into the best way to capture these biosamples i.e. what is the best ontology for cell lines.
• Current no column for deprecated or not. Not super relevant for current purposes of left-joining and wasn't entirely clear how to implement (drop deprecated, flag them, etc)
• dbXrefs currently a flat list of strings rather than split into ID and as done for variant index. This was partially for simplicity, partially because I was unsure how best to split (e.g. CL_1001593 -> [CL_1001593, CL], [CL_1001593, Cell Ontology], [1001593, CL], [CL_1001593, Uberon (ontology actually sourced from)] and partially because I didn't see an immediate use case unlike variant index.

[DSuveges]

This is not really good. We already have this column in other datasets eg. VariantIndex + others in the platform. The expectation from this filed is that it is a list of structs, where each element not only informs about the identifier, but also about the source.:

    {
      "metadata": {},
      "name": "dbXrefs",
      "nullable": true,
      "type": {
        "containsNull": true,
        "elementType": {
          "fields": [
            {
              "metadata": {},
              "name": "id",
              "nullable": true,
              "type": "string"
            },
            {
              "metadata": {},
              "name": "source",
              "nullable": true,
              "type": "string"
            }
          ],
          "type": "struct"
        },
        "type": "array"
      }
    }

[Tobi1kenobi]

I agree, sorta... See my comment on the pull request. It wasn't immediately apparent how to structure it

[DSuveges]

Great catch!

[DSuveges]

The configuration should have all the inputs that the step needs.

• cell_ontology_input_path
• uberon_input_path
• biosample_index_output_path

All can/should be missing in this case, because there's no hardcoded parameters eg. r2 threshold for LD index. Similarly, target_path needs to be dropped as this parameter is not part of the BiosampleIndexStep parameters.

[DSuveges]

This is just stylistic comment, but we do not import spark functions like this. There's a not too serious explanation for this: some of the functions might overwrite Python base function eg. from pyspark.sql.functions import sum, max, that's why we import all functions with a prefix:

import pyspark.sql.functions as f

# then calling a function would look like this:
f.array_distinct(f.array_union(f.col('colname')....

It's quite marginal, but being stylistically consistent is good.

[DSuveges]

I wouldn't add this function here. The merge of biosample indices is kind of an inherent ability of biosample indices as well. Eg.

a: BiosampleIndex
b: BiosampleIndex
merged: BiosampleIndex = a.merge_indices(b)

Assuming the operation is commutative, and b.merge(a) yields the same index.

So my advise is to move this logic as a instance method of the BiosampleIndex dataset.

[DSuveges]

(also this would imply one fewer function to import, as the merge function travels wherever the biosample object goes)

[DSuveges]

So this function is not entirely commutative, but I think this is still fine.

[DSuveges]

It's a great function, nice elegant logic! A few comments:

• I would add a description explaining that the order of element are not kept (lists are ordered structures).
• It returns with a unique set of elements, instead of all elements.
• I would put this function under common.utils.py, because such function might be useful in other places.

Question: Should this function be generalised to flatten arbitrarily deeply nested lists? (via recursion?)

[DSuveges]

Oh, if you want to flatten an array of array column in pyspark, you shouldn't use udfs! (in general udfs should be avoided if possible, because the interface between python and java layer makes the execution very inefficient and poorly scalable)

It works like this:

f.flatten(f.col('array_of_array_column"))

[DSuveges]

If you need a unique set of elements from the flattened array you can use:

f.array_distinct(f.flatten(f.col('array_of_array_column")))

[DSuveges]

As new parameter is added to the validation step, this new parameter needs to be added to the config file here.

[DSuveges]

This is not a critique, rather a comment: we are doing exactly the same steps for genes (disease validation is more complicated). I'm wondering if we can have one function that would be used by both the biosample and gene validation... eg. We would pass the following parameters to this function:

• this is the list of fields you need to find in
• this column, and if you don't find, add
• this flag...

Not exactly sure how to implement this, but sounds quite abstract that could be used in other places and other datasets maybe.