Bug fix: OMIMPS URI prefix #109
Conversation
joeflack4
force-pushed
the
omimps-prefix
branch
from
a84ef40
to
8d4453d
Compare
| @@ -77,7 +77,7 @@ | |||
| # LIDIA seems retired. so these are not resovable # Also: http://www.vetsci.usyd.edu.au/lida/ | |||
| 'LIDA': 'http://sydney.edu.au/vetscience/lida/dogs/search/disorder/' # Listing of Inherited Disorders in Animals (defunct?) | |||
| 'OMIM': 'https://omim.org/entry/' # Online Mendelian Inheritance in Man (human disease and variants) | |||
| 'OMIMPS': 'https://www.omim.org/phenotypicSeries/PS' # Online Mendelian Inheritance in Man (phenotypes) | |||
| 'OMIMPS': 'https://omim.org/phenotypicSeries/PS' # Online Mendelian Inheritance in Man (phenotypes) | |||
There was a problem hiding this comment.
The change
Updated the OMIMPS URI prefix to comply with what is in mondo-ingest.
Background
OK, so I was surprised to see this. This change had already been made in #107 (comment) on Feb 3rd, which I noticed and commented on.
But Nico undid those changes in a commit to main on Feb 7th.
Then, Sabrina noticed an issue on Feb 12th monarch-initiative/mondo-ingest#438.
I didn't get to work on fixing this until Mar 29th monarch-initiative/mondo-ingest#478. By that time, I'd forgotten about Nico having undone the change on Feb 7th.
What I'm not understanding though is how that PR was able to fix Sabrina's issue. Because that PR removed www from the URI in mondo-ingest. However, www remained in the omim repository.
I thought that maybe there was an OMIM release between Feb 3rd and Feb 7th, but I checked and there is none; only one release on the 7th, with Nico's commit message showing he'd added back www. So I have no idea how this is the case... that Sabrina's issue manifested... unless maybe there was actually a release in between those dates that was later deleted. Additionally, I don't know how my pull request was able to fix this issue. Rather, it should have broken things.
QC
- @matentzn @twhetzel Confirm that you like this strategy, or advise otherwise
- @joeflack4 Do the QC strategy
I think that some QC is needed here before merging. How does this plan sound?:
- I run an
omimrelease using this branch. - I run a build of
mondo-ingestfrom this branch where the URI prefix also needed to be updated: Bug fix: OMIMPS URI prefix mondo-ingest#504. Or I could runsh run.sh make slurp-omim. It will use that release. - I check the
slurp/omim.tsvand make sure that everything looks OK. - Merge this PR and Bug fix: OMIMPS URI prefix mondo-ingest#504
There was a problem hiding this comment.
I cant be of help here, other than saying that I undid that change in February for fear of unknown downstream repercussions - which I myself did not want to solve :P Sorry for this not-helpful comment
There was a problem hiding this comment.
@joeflack4 with the change you propose here for the OMIMPS URI, did you run through the QC steps you listed?
There was a problem hiding this comment.
That would be steps (1) through (3) above. Not yet. I'll tick off the box and also comment here when I go through these steps, assuming everything passes.
There was a problem hiding this comment.
Ok, I'd test this change to make sure the data that was problematic previously actually is fixed by this change given the churn of the history of what you detailed in the "Background".
There was a problem hiding this comment.
If you both are interested in a proper deep dive of the issue, I recommend to read this: biopragmatics/bioregistry#497
Just my two cents: I do not share the urgency with Chris and other community member to getting this fixed, but I guess its your turn now to decide these kinds of things! Happy to discuss it also in our next meeting
There was a problem hiding this comment.
@matentzn That is sorta unrelated to this thread, which is just about running QC on the change regarding www removal.
But oh yes, I will give that a proper read if / when I work on this. I created an issue for this for later:
There was a problem hiding this comment.
I ran the mondo-ingest build, but it's erroring out with a lexmatch error. I created an issue for that:
However, I did take a look at the slurp/omim.tsv before and after the changes in this PR; that's my main barometer for gauging if this change is working fine, and so far it looks good to me. I'll likely check the lexmatch outputs as well. Lemme know if you guys have any other ideas for QC checking.
I also ran the lexmatch on this build clone using my debugger, which was using an older version of oaklib which circumvented the error. Doing so allowed me to get these outputs, in which I see that OMIMPS appears:
lexmatch outputs.zip
There was a problem hiding this comment.
Build success & my QC check passed
I did a build with no errors:
My QC check is simply seeing that OMIMPS entries still appear in slurp/omim.tsv and mondo-sources-all-lexical.sssom.tsv. The comment above includes these outputs and, though they were ran and attached before #520 completed, I just looked at the output files in that PR and they also look good.
Given that, I'm happy to now to bring closure to the following PRs:
… is good for standardization reasons, and necessary to comply w/ what the URI is set to in the rest of the mondo-ingest pipeline.
joeflack4
force-pushed
the
omimps-prefix
branch
from
8d4453d
to
2925002
Compare
Changes
Additional info
This manifested in the prefix not getting collapsed in omim.ttl. I'm not sure if there were any other consequences aside from that on the
omimrepo side. But I would imagine that having the URL out of sync with what is inmondo-ingestis breaking something, though I'm surprised we would not have noticed yet.Related: