feat(l2g): add LocusToGeneCrossValidationStep for pre-split CV with hyperparameter tuning

[addramir]

✨ Context

Cross-validation and hyperparameter sweeps are exploratory — they should never run as part of a production training job. `LocusToGeneStep` previously had a `cross_validate` flag that was never used in prod and bloated its interface. This PR introduces a dedicated `LocusToGeneCrossValidationStep` that takes pre-built, annotated train/test feature matrix parquets and runs k-fold CV without any gold-standard parsing or feature matrix construction.

🛠 What does this PR implement

New: `LocusToGeneCrossValidationStep` (`l2g.py`)

A standalone step for CV and hyperparameter exploration. Takes two parquet inputs — a pre-split training set and a held-out test set, both annotated with features and `goldStandardSet` labels — and runs k-fold CV on the training partition followed by a final evaluation on the test set.

Parameters:

• `train_feature_matrix_path` / `test_feature_matrix_path` — required parquet paths
• `hyperparameters` — base XGBoost config
• `features_list` — defaults to the standard L2G feature list
• `n_splits` — CV folds (default 5)
• `hyperparameter_grid` — optional W&B-style grid `{"param": {"values": [...]}, ...}`; when combined with `cv_results_dir`, all configs are evaluated
• `cv_results_dir` — local or `gs://` path; when set, writes `cv_results.json`, `cv_folds.csv`, and per-config ROC / PR / confusion-matrix PNGs; without it, metrics go to the terminal

No W&B logging — CV output is file-based or terminal only.

Registered as Hydra step `locus_to_gene_cross_validation`.

New: `LocusToGeneTrainTestSplitStep` (`l2g.py`)

A standalone step for creating the train/test split upstream of CV or training. Registered as Hydra step `locus_to_gene_train_test_split`.

`LocusToGeneTrainer` upgrades

• `evaluate()` — extended with `TP`, `FP`, `TN`, `FN` confusion-matrix components
• `cross_validate()` — new `cv_results_dir` parameter enables file-mode output with explicit grid iteration
• `_run_cv_fold()` — extracts single-fold logic; adds locus-level resolution stats (`n_loci`, `n_loci_one_gene_above`, `n_loci_no_gene_above`)
• `_expand_grid()` — static method expanding a W&B-style grid into a flat list of hyperparameter configs
• `_summarise_and_plot_config()`, `_write_cv_files()`, `_upload_dir_to_gcs()` — file-mode output pipeline
• `_plot_roc_curves()`, `_plot_pr_curves()`, `_plot_confusion_matrix()` — per-config diagnostic PNGs

`LocusToGeneStep` refactor

Wired to consume pre-split train/test parquets from `LocusToGeneTrainTestSplitStep` instead of building the feature matrix inline. Gold-standard parsing and the `cross_validate` flag are removed from this step.

Config deduplication

Extracted `_L2G_FEATURES_LIST` and `_L2G_HYPERPARAMETERS` as module-level constants; `LocusToGeneConfig`, `LocusToGeneTrainTestSplitConfig`, and `LocusToGeneCrossValidationConfig` all reference them.

🧪 Test plan

• `pytest tests/gentropy/method/test_l2g_trainer.py` — all existing tests pass; `test_evaluate_perfect_predictions` updated for new TP/FP/TN/FN keys; `_expand_grid` and `_write_cv_files` unit tests added
• `pytest tests/gentropy/test_l2g_cv_step.py` — CV step integration tests including `cv_results_dir` file output and hyperparameter grid
• Smoke-test `LocusToGeneCrossValidationStep` with local parquets, terminal mode
• Smoke-test with `cv_results_dir` set to a local path and verify JSON/CSV/PNG outputs
• Smoke-test with `hyperparameter_grid` + `cv_results_dir` to verify grid expansion

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds a dedicated LocusToGeneCrossValidationStep that runs k-fold CV (with optional hyperparameter grid sweeps) on pre-built, annotated train/test feature-matrix parquets, leaving the production LocusToGeneStep untouched. The trainer is extended to support a file-output CV mode (cv_results_dir) producing JSON/CSV summaries and per-config ROC/PR/confusion plots, plus confusion-matrix counts in evaluate().

Changes:

• New LocusToGeneCrossValidationStep in l2g.py and matching LocusToGeneCrossValidationConfig registered under step=locus_to_gene_cross_validation.
• LocusToGeneTrainer.cross_validate refactored: _run_cv_fold extracted, hyperparameters now applied before fit (previously applied post-fit in W&B mode), cv_results_dir file-output mode with grid expansion and plotting helpers added.
• evaluate() extended with TP/FP/TN/FN; test updated accordingly.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
src/gentropy/l2g.py	New LocusToGeneCrossValidationStep that loads pre-split parquets, manually populates the trainer, runs CV, fits, and reports hold-out metrics.
src/gentropy/method/l2g/trainer.py	Refactored cross_validate with cv_results_dir file mode, _run_cv_fold, _expand_grid, plotting helpers, GCS upload helper, and confusion-matrix counts in evaluate.
src/gentropy/config.py	Added LocusToGeneCrossValidationConfig (duplicates feature list and hyperparameters from LocusToGeneConfig) and registered the Hydra step.
tests/gentropy/method/test_l2g_trainer.py	Updated test_evaluate_perfect_predictions for the new TP/FP/TN/FN keys.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[addramir]

Fixed: extracted _L2G_FEATURES_LIST and _L2G_HYPERPARAMETERS as module-level constants in config.py. All three configs (LocusToGeneConfig, LocusToGeneTrainTestSplitConfig, LocusToGeneCrossValidationConfig) now reference them via list(_L2G_FEATURES_LIST) / dict(_L2G_HYPERPARAMETERS).

[addramir]

Won't fix: the CV step is intentionally exploratory — its purpose is to evaluate configurations and write metrics, not to select and apply a 'best' config automatically. The caller inspects the output and chooses hyperparameters manually before running a production training job.

[addramir]

Fixed: added unpersist() calls on both feature matrix DataFrames immediately after .toPandas() materialises them.

[addramir]

Won't fix: the per-config summaries are intended for exploratory comparison across grid configurations, not fold-to-fold inference. Reporting mean ± std over all metrics (including counts) is acceptable in this diagnostic context.

[addramir]

Fixed: _upload_dir_to_gcs now delegates to copy_to_gcs from gentropy.external.gcs, removing the inline google.cloud.storage import and manual bucket/blob parsing.

[addramir]

Already covered: _expand_grid has unit tests in tests/gentropy/method/test_l2g_trainer.py (including valid grid, single-param grid, and invalid-key error cases). _write_cv_files and cv_results_dir file-mode output are tested in both test_l2g_trainer.py and tests/gentropy/test_l2g_cv_step.py.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

[addramir]

Partially addresed. I'd skip the n_splits=0 guard — it's not a realistic mistake and adding validation for every degenerate integer just adds noise. The holdout_only one is worth
it because it's a named flag with documented semantics that silently breaks.

[project-defiant]

Given refactor, we should move this upstream to the step. Feels awkward that cv function need to define by itself the parameter grid.

[project-defiant]

New parameter, did you run hyperparameter tuning?

[project-defiant]

As above, is there a hyperparameter tuning for gamma and max_delta_step ?

From xgboost:

gamma ∈ [0, ∞)
gamma = 0

Since gamma is the minimum loss reduction required for a split, the default (0) allows any split with positive regularized gain. This can produce more complex individual trees, including branches with small objective improvements

Again for the max_delta_step this is actually non-default value.

[project-defiant]

I think we should state what is the order of precedence for the hyperparameters.

1. if self.hyperparameters are a dict -> return them
2. else return dict(self.hyperparameters.default_factory())

Right now the execution is:
if self.hyperparameters == None:
raise
if self.hyperparameters is dict:
return them
else (case when hyperparameters are not null, nor a dict -> could be any other type):
return defaults.

[project-defiant]

It is a bit confusing that we have hyperparameters and hyperparameter_grid defined at the same time. I would probably keep only hyperparameter_grid as this is a superset of all parameter ranges, and given CV run, you will probably also want to sweep over the hyperparameter grid anyway.

On the second note, since we are not explicit on what kind of hyperparameters are expected (we pass them through directly to the xgboost model, it would be useful to have some minimal hint about that in the docstring + meaningful type hints). All parameters are expected to be of a metaType

class ParameterRange(NamedTuple):
    values: list[int] | list[float]
    name: str

class ParameterRangeSpec(NamedTuple):
    upper_bound: int| float
    lower_bound: int | float
    step: int | float

    def to_grid(name: str) -> ParameterRange:
        return ...

class Hyperparameters:
    grid: list[ParameterRange]

Given such types, we could easily understand the concept behind the input values.

[project-defiant]

FUN FACT -> https://arxiv.org/abs/2109.08203

The random seed probably should be handled as all other hyperparameters 😵, otherwise we could end up in non-optimal spot.

[project-defiant]

as above.

[project-defiant]

Since we are refactoring this bit, it would be useful if we specify one hyperparameter_grid for the entire step and reuse it everywhere (another words -> trainer.cross_validate should not need to look up for the trainer.hyperparameters and specifying the artificial grid if hyperparameter_grid is passed as None to the cross_validate method)

The handling of the hyperparameter_grid should happen before cross_validate call.

[project-defiant]

specifying hyperparameter_grid and hyperparameters feels redundant and implies implicit behavior which is not documented.

[project-defiant]

if we are not using the w&b for CV runs, we should remove the parameter and downstream logic.

[project-defiant]

Given refactor, we should move this upstream to the step. Feels awkward that cv function need to define by itself the parameter grid.