Skip to content

Fix parse_doi: DOI lost when a non-doi ELocationID (e.g. pii) follows it - #177

Open
ErisonChen wants to merge 1 commit into
titipata:masterfrom
ErisonChen:fix/doi-overwritten-by-trailing-elocationid
Open

Fix parse_doi: DOI lost when a non-doi ELocationID (e.g. pii) follows it#177
ErisonChen wants to merge 1 commit into
titipata:masterfrom
ErisonChen:fix/doi-overwritten-by-trailing-elocationid

Conversation

@ErisonChen

Copy link
Copy Markdown

Problem

parse_doi in medline_parser.py loops over every <ELocationID> and unconditionally reassigns doi on each iteration. Any ELocationID whose EIdType is not "doi" resolves to "". So when a record lists the doi entry before another type (commonly pii), the trailing entry overwrites the already-parsed DOI with an empty string.

Real example — PMID 24746000:

<ELocationID EIdType="doi" ValidYN="Y">10.1016/S2213-2600(14)70019-0</ELocationID>
<ELocationID EIdType="pii" ValidYN="Y">S2213-2600(14)70019-0</ELocationID>

parse_medline_xml returns doi == "" instead of 10.1016/S2213-2600(14)70019-0. This doi-then-pii ordering is very common in PubMed, so a large number of records silently lose their DOI.

There is also a second, related bug: the PubmedData/ArticleIdList fallback is in the else branch of if len(elocation_ids) > 0, making it unreachable whenever any ELocationID is present — even if none of them is a DOI (e.g. a record that only has a pii ELocationID but carries the DOI in ArticleIdList).

Current code

if len(elocation_ids) > 0:
    for e in elocation_ids:
        doi = e.text.strip() or "" if e.attrib.get("EIdType", "") == "doi" else ""
else:
    article_ids = pubmed_article.find("PubmedData/ArticleIdList")
    ...

Fix

  • Iterate ELocationID and stop at the first EIdType="doi" entry, so a later non-doi entry can no longer clobber it.
  • Always fall back to PubmedData/ArticleIdList when no DOI was found in the ELocationID list (covers records that only have e.g. a pii entry).

Tests

Added a regression test (test_doi_with_trailing_pii_elocation_id) using PMID 24746000, whose <Article> lists doi before pii. It fails on the current code and passes with the fix.

Verified the new logic against four cases — doi-before-pii, pii-only with DOI in ArticleIdList, pii-before-doi, and no-DOI-at-all — all behave correctly, and the no-DOI case still returns "".

`parse_doi` looped over every <ELocationID> and unconditionally
reassigned `doi` each iteration, with non-"doi" types resolving to "".
When a record lists the "doi" ELocationID before another type (e.g.
"pii"), the trailing entry overwrote the DOI with an empty string.

Example (PMID 24746000):

    <ELocationID EIdType="doi" ...>10.1016/S2213-2600(14)70019-0</ELocationID>
    <ELocationID EIdType="pii" ...>S2213-2600(14)70019-0</ELocationID>

parsed to doi="" instead of the real DOI. This ordering is common in
PubMed, so many records lost their DOI.

Additionally, the `ArticleIdList` fallback lived in the `else` branch of
`if len(elocation_ids) > 0`, so it was unreachable whenever any
ELocationID existed -- even if none of them carried a DOI.

Fix:
- iterate ELocationID and stop at the first EIdType="doi" entry, so a
  later non-doi entry can no longer clobber it
- always fall back to PubmedData/ArticleIdList when no DOI was found in
  the ELocationID list (covers records that only have e.g. a pii entry)

Add a regression test using PMID 24746000 (doi-before-pii ordering).
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.

1 participant