fix(qc): raise DUPLICATED_STUDYLOCUS_ID and make duplicate flagging deterministic

[addramir]

Problem

StudyLocus.validate_unique_study_locus_id exists and is unit-tested, but it is never called anywhere in the codebase:

$ grep -rn "validate_unique_study_locus_id" src/
src/gentropy/dataset/study_locus.py:388:    def validate_unique_study_locus_id(...)

So DUPLICATED_STUDYLOCUS_ID is never raised on any dataset. Release configs list it in the credible_set_validation step's invalid_qc_reasons, which currently has no effect — non-unique studyLocusIds pass straight through into output/credible_set.

Found while ingesting an external meta-analysis: a delivery whose credible sets contained 212 rows for 145 distinct signals (overlapping ±1 Mb fine-mapping windows re-fine-mapped the same region) produced 212 rows carrying only 144 distinct studyLocusIds, and the validation step raised nothing. Deduplication had been deliberately deferred to that step on the assumption the flag was live.

A second, smaller issue: Dataset.flag_duplicates ordered its window by an unseeded rand(), so which duplicate was retained varied between runs over identical input.

Changes

StudyLocusValidationStep now calls validate_unique_study_locus_id(). This step is the right place for the check: its input is a union of independently generated credible set datasets (gcca, gcss_pics, gcss_susie, finngen, eqtl_catalogue, ukb_ppp_eur), so cross-source uniqueness can only be established once they are combined. SUSIE_inf.credible_set_qc already deduplicates, but only within a single fine-mapping output and by dropping rows rather than flagging them.

Dataset.flag_duplicates orders by monotonically_increasing_id() instead of rand(), and takes an optional order_by so callers can state which duplicate should win.

Behaviour

Flag-only, no rows dropped — as with every other check in the step. Datasets that already have unique ids are unaffected. Datasets that do not will see DUPLICATED_STUDYLOCUS_ID on all but one row per id, and those rows move to excluded/credible_set under the existing release config. That is the intent of the config, but it is a live-output change worth a reviewer's attention.

Tests

tests/gentropy/dataset/test_dataset.py:

• test_flag_duplicates_is_deterministic — repeated runs retain the same row
• test_flag_duplicates_honours_order_by — caller ordering decides the survivor

The existing TestStudyLocusDuplicationFlagging already covers the flagging logic itself.

Notes for reviewers

• StudyIndex.validate_unique_study_id has the same problem — defined and tested, but not called by StudyValidationStep. Left out to keep this focused; happy to fold it in.
• flag_duplicates retains an arbitrary-but-reproducible row. For credible sets, keeping the highest credibleSetlog10BF would be more meaningful and would match SUSIE_inf.credible_set_qc; that is a one-line order_by at the call site if you want it here.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR activates duplicate studyLocusId QC flagging during the credible set validation workflow and makes duplicate-flagging deterministic by removing rand()-based ordering.

Changes:

• Call StudyLocus.validate_unique_study_locus_id() from StudyLocusValidationStep so DUPLICATED_STUDYLOCUS_ID is actually emitted during validation.
• Update Dataset.flag_duplicates to use a deterministic default ordering (monotonically_increasing_id()) and add an optional order_by override for callers.
• Add unit tests ensuring duplicate flagging is deterministic and respects caller-provided ordering.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
tests/gentropy/dataset/test_dataset.py	Adds regression tests for deterministic duplicate flagging and order_by-controlled survivor selection.
src/gentropy/study_locus_validation.py	Wires validate_unique_study_locus_id() into the study locus validation pipeline so duplicate IDs are flagged.
src/gentropy/dataset/dataset.py	Makes flag_duplicates deterministic and adds an optional ordering parameter.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[project-defiant]

In general I agree with the decisions. Please have a look at the comments. No deal breaker.

[project-defiant]

This is more a comment on the design, rather then on the code.

There can be two duplicate examples:

1. two credible sets arose from the same locus - exactly the same, hence we can just keep one and remove others via flagging (which you refactored)
2. two credible sets are not the same, even though they have the same studyLocusId (we do not use entire locus object as a studyLocusId discriminator, hence we can have a mismatch). This case probably kills the idea of studyLocusId in the first place, as it could become non-unique.

I am not convinced if we can just deal with 1) but not check 2) here and arbitrary choose one of loci.

[project-defiant]

Hence it is a library function, please add an example usage.

[project-defiant]

I am not sure if using the monotonically_increasing_id do anything here. The order should be the same (read from top to bottom at random, depending on how the data is shuffled before the partition by test column)

[project-defiant]

I am not sure I follow this, is it meant to test the resilience of monotonically_increasing_id with multiple retained() calls ? This would make sense for such a small scale dataset on test run at single machine, but will not be guaranteed on the cluster at all.