enrichex/qtlcascade: duplication left behind by the #303 de-duplication pass
- 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
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