Skip to content

refactor(clinical_report): clean id signatures - #75

Open
ireneisdoomed wants to merge 6 commits into
mainfrom
il-4477
Open

ireneisdoomed wants to merge 6 commits into
mainfrom
il-4477

Conversation

@ireneisdoomed

Copy link
Copy Markdown
Contributor

Issue

After @remo87 investigation, he observed that the root cause for opentargets/issues#4477 is the fact that some clinical report ids present unescaped characters that make the API fail. For example, d00ijm/friedreich's ataxia has the ' character unscaped. That means that if you request any drug information where that clinical report is supporting the association, the whole query will fail. There are 27 of such IDs

The fix

  1. clinical_report.sanitise_text has code to clean unescaped characters from any string columns in the root level. Unfortunately, ' wasn't part of the regex. A quick solution to this issue is to include ' in the regex

  2. The offending clinical report IDs come from TTD. TTD presents information at the drug level. How we convert this into clinical report evidence is by creating a row for each drug/indication. We literally create the ID by concatenating the drug and the indication. That is how this issue originated.
    Because this is an artificial ID that doesn't directly reflect a reference in the primary source, I think this can be improved. We already have examples of hashed IDs.

What this PR implements is a rule where the ID is only kept if the reference is meaningful in the context of the primary source. For example, an NCT ID, an EMEA reference, a Dailymed ID,... all of these can be looked up in the primary source. However, when the ID is artificial, we hash it.

In practice this has only changed the way we assign IDs for TTD and ChEMBL. But the above rule should be maintained keeping forward.

Let me know what you think

@ireneisdoomed

Copy link
Copy Markdown
Contributor Author

This one would be nice to address for PPP, but it is not crucial

@d0choa d0choa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at this closely since it changes public IDs. The hashing approach is right and the rule you've written down is a good one to keep.

The description doesn't match the diff. Fix #1 (adding ' to the regex) isn't here — sanitise_text is untouched — and wouldn't have worked anyway: it's an unescaper (\xx), so it'd only catch \', not a bare apostrophe. The 27 IDs are still fixed, via SHA-256 on the TTD side. Worth correcting the description.

One ask — make the ChEMBL decision an allowlist (chembl/indications.py:99). The .otherwise keeps the raw ref_id for any ref_type outside ["INN","FDA","USAN"], so a new ChEMBL ref_type would silently pass its raw ID through — the exact failure class this PR closes. Flipping it makes the rule self-enforcing:

id=(
    # Meaningful references are kept as-is: they can be looked up in the primary source
    pl.when(pl.col("source").is_in(["ClinicalTrials", "DailyMed", "EMA", "ATC"]))
    .then(pl.col("id"))
    # Anything else is an artificial ID and gets hashed
    .otherwise(pl.col("id").chash.sha2_256())
)

One question: FDA is hashed, but it sits in APPROVAL_SOURCES alongside ATC/EMA/DailyMed, which you're deliberately not hashing. Is an FDA ref_id not lookup-able the way those are? Not blocking, just couldn't tell if it's intentional.

Last thing — this changes IDs for all TTD and ChEMBL INN/FDA/USAN rows, not just the 27. Worth a line in the PR body since they're API-facing (url is preserved, so provenance survives).

@ireneisdoomed

Copy link
Copy Markdown
Contributor Author

Thanks for the comments, @d0choa !

The description doesn't match the diff.

  1. I guess my PR description is not AI interpretable... The initial comment was that I could see 2 options: 1) ad hoc clearance in the sanitise function; 2) hashing IDs. I implemented option 2

make the ChEMBL decision an allowlist

  1. I partly disagree on this one. I am deliberately referencing those sources we've hashed so the behaviour is clearer because it is easy to know that Dailymed or EMA indications refer to their own internal ID, but the definition for the INN references is trickier. I prefer listing the rarer cases rather than saying Anything else. I could also be fully transparent and combine both in two separate when clauses.

  2. Good call here; I forgot to add this as a note. I had doubts about the interpretation of FDA IDs... So for these cases, ref_id can represent:

  • Drug approval references like 021409orig1s045lbl.pdf or label/2019/211996s000 (~500 reports)
  • Dailymed IDs (for some reason) (~1000 reports)
    Because the reference to these labels are in the url, I decided to remove the variance by hashing. Happy to revert, both is fine.
  1. Can you specify? I've mentioned already in the PR description that this spans the 27 rows and affects TTD and ChEMBL references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants