Migrate gene_cnv() from AnophelesCnvFrequencyAnalysis to AnophelesCnvData (resolves dipclust.py TODO)

[khushthecoder]

Summary

There is a TODO in malariagen_data/anoph/dipclust.py indicating that gene_cnv() still needs to be migrated into the modular CNV “data-layer” mixin architecture used by the rest of malariagen_data/anoph/.

Specifically, gene_cnv() is currently implemented in malariagen_data/anoph/cnv_frq.py (as part of AnophelesCnvFrequencyAnalysis) but is conceptually a data access method (returns an xr.Dataset of modal copy number by gene). This creates architectural inconsistency and forces dipclust.py to call it in a way that bypasses the standard class-hierarchy pattern.

How to generate this issue

1. Open malariagen_data/anoph/dipclust.py.
2. Jump to the TODO block at ~lines 401–404:
   • It explicitly says gene_cnv() needs to be migrated to the AnophelesCnvData class so it can be found in the class hierarchy.
3. Confirm where gene_cnv() lives today:
   • Search in malariagen_data/anoph/ and you’ll find gene_cnv() implemented in malariagen_data/anoph/cnv_frq.py.
   • Verify that malariagen_data/anoph/cnv_data.py (which defines AnophelesCnvData) does not contain gene_cnv().

This mismatch between dipclust.py (TODO + type ignore) and the actual method location is the root problem.

Problem / Current state

• dipclust.py contains:
  • a TODO noting gene_cnv() must be migrated to AnophelesCnvData
  • a self.gene_cnv(...) # type: ignore call inside _dipclust_cnv_bar_trace()
• gene_cnv() is implemented in malariagen_data/anoph/cnv_frq.py (within AnophelesCnvFrequencyAnalysis), not in malariagen_data/anoph/cnv_data.py (AnophelesCnvData).

As a result:

• The API surface is inconsistent: a “gene copy number data access” method is tied to a “frequency analysis” class rather than the CNV data mixin.
• Some analysis modules/components rely on inheritance via specific analysis classes instead of the intended data-layer hierarchy.
• Type-checking / linting hints (the # type: ignore) show the hierarchy is not clean for method discovery.

Why this is important (Impact)

This is important because it increases fragility and maintenance cost:

• Architectural consistency: Other CNV “data” methods live in cnv_data.py under AnophelesCnvData, but gene_cnv() does not—this breaks the intended pattern.
• Onboarding & extensibility: Adding or reusing gene-level CNV datasets in new modules becomes harder because developers must know that gene_cnv() lives in an analysis mixin rather than the data mixin.
• Inheritance correctness: If a future class inherits only AnophelesCnvData (and not AnophelesCnvFrequencyAnalysis), it will not naturally get gene_cnv(), even though it is a data access method.
• Maintainability: Moving logic into the correct layer reduces the chances of duplicated logic and future “workarounds” (type: ignore, direct calls, or special-case imports).

Proposed fix

1. Move gene_cnv() (and its internal helper, e.g. _gene_cnv)
   • From malariagen_data/anoph/cnv_frq.py
   • Into malariagen_data/anoph/cnv_data.py
   • As methods on AnophelesCnvData.
2. Update callers to use the standard hierarchy
   • malariagen_data/anoph/cnv_frq.py:
     • Adjust AnophelesCnvFrequencyAnalysis so it calls the moved gene_cnv() implementation (directly or via a shared helper).
   • malariagen_data/anoph/dipclust.py:
     • Remove the TODO and remove the # type: ignore if possible after the method is available on the proper class hierarchy.
3. Keep public behavior stable
   • Preserve the existing gene_cnv() signature and returned dataset structure so downstream plots/analyses don’t break.

Tests / Acceptance criteria

• Existing CNV/dipclust-related test suites pass.
• Add/adjust unit tests to verify:
  1. AnophelesCnvData exposes gene_cnv() (or that instances in the expected hierarchy do).
  2. AnophelesDipClustAnalysis no longer needs the # type: ignore for self.gene_cnv(...) (or at least no runtime break occurs).
  3. Output schema of gene_cnv() is unchanged (coords/data_vars expected by downstream code).

Implementation approach (high level)

• Extract the current gene_cnv() / _gene_cnv code from cnv_frq.py.
• Paste into AnophelesCnvData in cnv_data.py with minimal changes.
• Update imports so cnv_data.py has access to whatever dependencies gene_cnv() currently uses (e.g., Region, _parse_multi_region, _cn_mode, genome feature access, etc.).
• Refactor AnophelesCnvFrequencyAnalysis to call self.gene_cnv(...) from the data mixin.
• Update dipclust.py to call gene_cnv() without the migration warning/type workaround.

Reference

• TODO location: malariagen_data/anoph/dipclust.py (~lines 401–404)
• Current implementation location: malariagen_data/anoph/cnv_frq.py
• Intended target location: malariagen_data/anoph/cnv_data.py (AnophelesCnvData)

[khushthecoder]

Update: I'm currently working on a fix for this. Please check back before opening a PR — I'll post a link here once it's ready. Thanks!

[kunal-10-cloud]

Hi @khushthecoder i am interested in contributing to this issue if you dont mind can i take up this one

[khushthecoder]

Hi @kunal-10-cloud , sure go ahead

[kunal-10-cloud]

Why This Is Important:

The gene_cnv() method is fundamentally a data access operation—it retrieves CNV data from the genome and formats it into a structured dataset. Currently, it lives in the analysis layer, but according to our architectural pattern, data access methods belong in the data layer. This violates our modular design principle and creates confusion about where data operations actually live.

Impact:

This architectural inconsistency makes the codebase harder to navigate and maintain. It also forces downstream analysis methods to inherit data operations through an analysis class rather than directly from the data layer—a semantic mismatch. There's even a [type:ignore] comment in one of the callers that exists precisely because of this misplacement.

Approach:

We move gene_cnv() and its supporting functions from the analysis layer to the data layer. This restores proper architectural alignment—analysis classes inherit data methods through the natural hierarchy. All existing code continues to work seamlessly, we remove the type-checking workaround, and all tests pass without modification. It's fully backward-compatible while significantly improving code clarity.

Hi @jonbrenas, i have made a PR(#1214 ) for this issue after understanding the context of it with @khushthecoder
Please review it once whenever you are available, and let me know if any changes are required

[jonbrenas]

Hi @kunal-10-cloud, @khushthecoder, I think something got missed about the reason of the movement (at least from what I see in #1214. Why, do you think, the comment says that this function should be moved? What does it mean for the diplotype clustering class? Is it the only such function that needs to be moved?

[kunal-10-cloud]

Hi @jonbrenas, let me explain the reasoning:

Why should gene_cnv() be moved?

The TODO in dipclust.py (lines 401–404) highlights that gene_cnv() is a data-access method (it returns an xr.Dataset of modal copy number per gene from HMM data), but it currently lives in AnophelesCnvFrequencyAnalysis — a frequency analysis class.

The data-layer mixin pattern used throughout malariagen_data/anoph/ places data-access methods in *Data classes (like AnophelesCnvData), so gene_cnv() is architecturally misplaced. This forces dipclust.py to use self.gene_cnv(...) # type: ignore because the method isn't discoverable from the expected position in the class hierarchy.

What does it mean for the diplotype clustering class?

AnophelesDipClustAnalysis inherits from AnophelesCnvFrequencyAnalysis, which in turn inherits from AnophelesCnvData. After moving gene_cnv() down to AnophelesCnvData, it becomes available via the standard inheritance chain at the data-layer level.

This means:

• The # type: ignore comment and TODO in dipclust.py can be removed.
• Any future class that inherits only from AnophelesCnvData (without needing frequency analysis) will also have access to gene_cnv().
• Type checkers can properly resolve the method — no more workarounds.

Is it the only such function that needs to be moved?

From my review, gene_cnv() is the only method in cnv_frq.py that is purely data-access, and it is the only self. call in dipclust.py that requires # type: ignore due to this hierarchy issue.

The other methods in cnv_frq.py — gene_cnv_frequencies() and gene_cnv_frequencies_advanced() — are genuinely frequency-analysis methods (they compute cohort frequencies, amp/del counts, etc.), so they correctly belong in AnophelesCnvFrequencyAnalysis.

The _cn_mode/_cn_mode_1d numba helpers are moved alongside gene_cnv() since they're only used by _gene_cnv().

Happy to make any further adjustments if needed!

[jonbrenas]

Hi @kunal-10-cloud,

Thank you for trying to answer my questions. You got it (mostly) wrong.

The main technical motivation is that diplotype clustering is a statistic that uses CNV and SNP data, not CNV or SNP frequencies (and homozygosity, but that's a different problem). Currently, AnophelesDipClustAnalysis inherits from AnophelesCnvFrequencyAnalysis and AnophelesSnpFrequencyAnalysis, which is incorrect. gene_cnv could be called without issue because it is defined in AnophelesCnvFrequencyAnalysis, and it also work after it being moved because AnophelesCnvFrequencyAnalysis inherits from AnophelesCnvData. Does that make sense?

[kunal-10-cloud]

Hi @jonbrenas,

Thank you for the correction — that makes sense and I appreciate you taking the time to explain.

To make sure I understand correctly:

The core issue is that AnophelesDipClustAnalysis currently inherits from AnophelesCnvFrequencyAnalysis and AnophelesSnpFrequencyAnalysis, but it really only needs the data-access layers (CNV + SNP data), not the frequency analysis layers. The inheritance from the *FrequencyAnalysis classes is what's architecturally wrong — dipclust uses raw data (genotypes, copy numbers), not computed frequencies.

Moving gene_cnv() down to AnophelesCnvData is a prerequisite step that enables a future refactor where AnophelesDipClustAnalysis could inherit from the *Data classes directly instead of the *FrequencyAnalysis classes — which would make the inheritance chain accurately reflect what dipclust actually depends on.

The # type: ignore wasn't because gene_cnv() was unreachable at runtime (it worked fine via MRO through AnophelesCnvFrequencyAnalysis), but because the type checker couldn't cleanly resolve it — a symptom of the architectural mismatch.

So the migration in this PR is correct as a first step, but the bigger picture is fixing the inheritance of AnophelesDipClustAnalysis itself. Would you like me to look into what it would take to update the dipclust inheritance chain as a follow-up, or is that something you'd prefer to scope separately?

[jonbrenas]

Thanks @kunal-10-cloud.

Would you like me to look into what it would take to update the dipclust inheritance chain as a follow-up, or is that something you'd prefer to scope separately?

Yes, that would be great.