Codecov Report

Attention: Patch coverage is 97.78870% with 9 lines in your changes missing coverage. Please review.

Project coverage is 97.20%. Comparing base (ebb4384) to head (440a5ef).
Report is 559 commits behind head on master.

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
+ Coverage   97.07%   97.20%   +0.12%     
==========================================
  Files          24       26       +2     
  Lines        1673     2072     +399     
==========================================
+ Hits         1624     2014     +390     
- Misses         49       58       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Need to resolve _lookup_sample() in AnophelesCnvData.
I wonder if the plan is to move it from anopheles to anoph.base, to work in a similar fashion to lookup_release(). 🤔

Perhaps lookup_sample() belongs in AnophelesSampleMetadata instead.

Need to resolve the contig param of cnv_discordant_read_calls() in AnophelesCnvData.
Can't use base_params.contig because it needs to support a list as well as a str.
There is also a note that says:

# N.B., we cannot support region instead of contig here, because some
# CNV alleles have unknown start or end coordinates.

🤔

In order to cater for cnv_discordant_read_calls(), I wonder if we need to switch to using something similar to the region_param_type for contigs, e.g.

single_contig_param_type: TypeAlias = str

contig_param_type: TypeAlias = Union[
    single_contig_param_type,
    List[single_contig_param_type],
    Tuple[single_contig_param_type, ...],
]

Another approach or convention, which would be a breaking change, could be to use plural param names when either a string or a list or a tuple are accepted, e.g. contigs, and use singular when only a string, representing the singular, are accepted, e.g. contig.

Another approach or convention, which would be a breaking change, could be to use plural param names when either a string or a list or a tuple are accepted, e.g. contigs, and use singular when only a string, representing the singular, are accepted, e.g. contig.

This is tempting, but I think I'd prefer to avoid API change unless really necessary.

In order to cater for cnv_discordant_read_calls(), I wonder if we need to switch to using something similar to the region_param_type for contigs, e.g.

single_contig_param_type: TypeAlias = str

contig_param_type: TypeAlias = Union[
    single_contig_param_type,
    List[single_contig_param_type],
    Tuple[single_contig_param_type, ...],
]

This seems reasonable to me.

Need to resolve plot_cnv_hmm_coverage_track() use of the string "auto" for param y_max, which is usually a float.

Perhaps lookup_sample() belongs in AnophelesSampleMetadata instead.

SGTM.

It was easier to create a new base_param named contigs than it was to change all cases that use contig to use single_contig (there is only one case that supports a list of contigs), but this leads to a naming inconsistency with region. I am tempted to rename and refactor all cases of region to use regions instead. This would make the difference between singular and plural types more obvious, and consistent.

contig: TypeAlias = Annotated[
    single_contig_param_type,
    """
    Reference genome contig name. See the `contigs` property for valid contig
    names.
    """,
]

contigs: TypeAlias = Annotated[
    contig_param_type,
    """
    Reference genome contig name. See the `contigs` property for valid contig
    names. Can also be a sequence (e.g., list) of contigs.
    """,
]

single_region: TypeAlias = Annotated[
    single_region_param_type,
    """
    Region of the reference genome. Can be a contig name, region string
    (formatted like "{contig}:{start}-{end}"), or identifier of a genome
    feature such as a gene or transcript.
    """,
]

region: TypeAlias = Annotated[
    region_param_type,
    """
    Region of the reference genome. Can be a contig name, region string
    (formatted like "{contig}:{start}-{end}"), or identifier of a genome
    feature such as a gene or transcript. Can also be a sequence (e.g., list)
    of regions.
    """,
]

I am tempted to rename and refactor all cases of region to use regions instead. This would make the difference between singular and plural types more obvious, and consistent.

No objection. Would also then have consistency in type name with the sample_set and sample_sets types too.

I'm now wondering if it makes sense for cnv_data to be using het_params, or whether it would be better to promote these to base_params (or perhaps gplt_params), like single_sample was.

    def plot_cnv_hmm_coverage_track(
        self,
        sample: het_params.sample,
        region: base_params.region,
        sample_set: Optional[base_params.sample_set] = None,
        y_max: cnv_params.y_max = cnv_params.y_max_default,
        sizing_mode: gplt_params.sizing_mode = gplt_params.sizing_mode_default,
        width: gplt_params.width = gplt_params.width_default,
        height: gplt_params.height = 200,
        circle_kwargs: Optional[het_params.circle_kwargs] = None,
        line_kwargs: Optional[het_params.line_kwargs] = None,
        show: gplt_params.show = True,
        x_range: Optional[gplt_params.x_range] = None,
        output_backend: gplt_params.output_backend = gplt_params.output_backend_default,
    ) -> gplt_params.figure:

I am also slightly concerned about cnv_params now having a mixed-type y_max and a different y_max_default to het_params, although this avoids changing the API.

As suspected, having a mixed-type for y_max causes type-safety / inconstencey flags, e.g.

fig.yaxis.ticker = list(range(y_max + 1))

which would error if a string other than "auto" was used as param

        if y_max == "auto":
            y_max = data["call_CN"].max() + 2

🤔

It also looks like there is a problem re-using the het_params circle_kwargs for cnv_data, e.g.

circle_kwargs.setdefault("size", 3)

Raising no attribute "setdefault".

🤔

There is also an "incompatible types" issue in cnv_data's treatment of base_param sample_sets, i.e.

        if sample_sets is not None:
            if isinstance(sample_sets, (list, tuple)):
                sample_sets_text = ", ".join(sample_sets)
            else:
                sample_sets_text = sample_sets
            title += f" - {sample_sets_text}"

Somehow clashing with Union[Sequence[str], str],. I gather Sequence isn't actually an instance of a list or tuple, so gets treated as if it were a string in this logic.

I gather I need to improve test coverage on cnv_data.py from 65% to 97% to satisfy these CI checks.

• https://github.com/malariagen/malariagen-data-python/pull/411/checks?check_run_id=14772791285

Nearly there! 96.32% of diff hit (target 96.95%)

Ah, I suspected some of this and should have asked! Many thanks @alimanfoo . I'll see what I can do and might need to clarify some things next week.