bigbio / bigbio/hvantk

enrichex/qtlcascade: duplication left behind by the #303 de-duplication pass

Open
#308 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Python
Stars
1
Forks
0
Avg merge
1d 3h
Merged PRs (30d)
40

Description

#303 de-duplicated `_empty_figure` (into `visualization/base.empty_figure`) and `_resolve_significance`. The review found near-identical cases it did not cover.

## 1. `_resolve_pvalue_column` is still duplicated

```
hvantk/algorithms/enrichex/plot.py:1053:def _resolve_pvalue_column(df: pd.DataFrame) -> str:
hvantk/algorithms/enrichex/report.py:635:def _resolve_pvalue_column(df: pd.DataFrame) -> str:
```

Behaviourally identical, but `plot.py` iterates the module constant `_P_VALUE_COLUMNS` while `report.py` hardcodes the same tuple inline. This one matters more than it looks now that #303 made `_resolve_significance` shared: `report.py` derives `p_col` with its **local** resolver and feeds it to `plot.py`'s **imported** significance function, so the two halves of one decision come from two copies. Adding a column name (`q_value`, `fdr`) to `_P_VALUE_COLUMNS` would fix the plots and not the report, surfacing as a `KeyError` inside the shared helper.

Fix: add `_resolve_pvalue_column` to the existing `from hvantk.algorithms.enrichex.plot import (...)` block one line below the `_resolve_significance` #303 added, and delete `report.py:635`.

## 2. Two more copies of the placeholder figure

`hvantk/algorithms/qtlcascade/plot.py:87` and `:145` each inline the same idiom `empty_figure` now owns: `plt.subplots(figsize=...)` + centered `ax.text(0.5, 0.5, "No data", transform=ax.transAxes, ...)` + save + return. That module already imports from `visualization.base`, so the path is established.

They are not byte-identical to the consolidated helper, which is itself the problem: `empty_figure` renders "No data available" at fontsize 14 in `#888888` with ticks and spines stripped, while the qtlcascade copies render "No data" in default black with a full box and tick marks. The same empty-result state looks like two different products across `hvantk qtlcascade report` and `hvantk enrichex`.

## 3. Aliased import hides the move

`enrichex/plot.py` and `ptm/plot.py` both do `from ...base import empty_figure as _empty_figure` to avoid touching 19 call sites. The alias is now the only thing pointing at the old private name — `def _empty_figure` exists nowhere in the tree. Anyone grepping for it finds nothing, which is the confusion the de-duplication was meant to end. A mechanical rename at the 19 call sites would close it.

Contributor guide

Open the contributing guide

Research direction

Start with the cited _resolve_pvalue_column definitions in enrichex/plot.py and enrichex/report.py, then inspect visualization/base.empty_figure and the two qtlcascade/plot.py placeholder blocks. Search the tree for the 19 _empty_figure call sites and the aliased imports. Done means one shared p-value resolver, all empty-result figures use empty_figure, and the old private alias is gone.

Written by the indexing model from the issue text.

Assessment

Tech stack
pandas, python
Domain
data-visualization
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.