Feature/expand phenopackets - Builds on top of feature/preprocessing-columns

[VarenyaJ]

(phenopacket): Expand mapping to include diseases, measurements and biosamples

• Add DiseaseRecord, MeasurementRecord and BiosampleRecord dataclasses
• Extend loader and RENAME_MAP to recognize measurement and biosample columns
• Update DefaultMapper.apply_mapping to detect disease, measurement and biosample sheets by key columns
• Wire up CLI parse-excel to serialize diseases, measurements and biosamples blocks into each phenopacket JSON
• Add integration test test_full_features_parse_creates_all_blocks
• Clean up ruff warnings: remove unused records assignments in mapper stubs (for now)

[ielis]

The test receives a path to a decompressed HPO JSON file. However, HPO toolkit will decompress hp.json.gz on the fly (it picks up the .gz suffix).

So, you do not need to do the decompression, unless you really want to do it from some other reason.

[ielis]

I see a lint in my IDE - the method does not override the abstract method in the correct fashion.

It should return a typing.Sequence[Phenopacket], but it returns a tuple of other things.

We should either change the TableMappers signature or return a sequence of phenopackets from the overriding method (this method, preferred solution).

---

A second point. This function involves a lot of guesswork regarding finding out what sheet are we dealing with. This brings a lot of unnecessary complexity. I think it would be better to make this mapper work with a dict of data frames where the dict keys either include known values (genotype, phenotype, etc.) or we bail out. I do not think we should try to make this class to work for any set of data frames imaginable. That is just too hard to test and reason about.

Working with known table names allows us to delegate parsing of the genotype and phenotype into separate mappers, which we can test. The parsing can be followed with aggregation into phenopackets.

So, I would recommend to simplify the logic by basically asking for "standardized" table names.

---

A third point. I think this function does a lot of stuff and it may be worth to try to split it into smaller components, each responsible for doing one thing. For instance, in case of _check_hgvs_consistency, we can make it into a separate function that checks a single item instead of a frame of items. Something like:

def check_hgvs_consistency(
    item: pd.Series,
    notepad: Notepad,
):
    ...

where is a row with a variant to check.

Then you can write tests for all sorts of weird variants and know that the errors will be reported. Using the function from DefaultMapper will also be trivial; we can call it within a loop.

Similarly, we can isolate the logic for parsing genotypes (self._map_genotype) and phenotypes (self._map_phenotype) into their own components. The logic is anything but trivial. The same drill applies - make it work with a single data item, test it, and let the mapper orchestrate their application on the provided tables.

[ielis]

Hello Varenya,

I had a look at the code and I do not see any principal issues with the code logic. I would, however, recommend restructuring the code. The DefaultMapper is the most important pipeline component, and it should be split into several smaller components. Each of the component should deal with a particular task and be well tested.

I see that you have some top-level tests of the CLI, and those are great to ensure that CLI works as intended. It is hard, however, to test all sorts of weird situations that can arise when parsing variants or phenotypes (e.g. a non-existing HPO term, such as HP:9999999). If you split the mapper into smaller components (classes or functions), you will be able to thoroughly test them, and increase the confidence about the code.

I added several comments below (or above?).