Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multiple conditions parsing #115

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Conversation

northwestwitch
Copy link
Member

@northwestwitch northwestwitch commented Apr 23, 2024

This PR adds | fixes:

How to prepare for test:

  • Download the scout submission mentioned in the issue as json and convert MultipleConditionExplanation to lower case

How to test:

Expected outcome:

  • Validation should pass

Review:

  • Code approved by
  • Tests executed by CR

This version is a:

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

@northwestwitch
Copy link
Member Author

northwestwitch commented Apr 23, 2024

After downloading the submission in question from Scout:

  • Submission with uppercase (MultipleConditionExplanation)
image
  • After fixing the field name with lowercase (multipleConditionExplanation):
image

The remaining error in the validation is due to one of the variants that has an empty observedIn field. This variant in ANKRD11 has also other issues so better fixing it before doing the submission

image

@northwestwitch
Copy link
Member Author

Other errors I see in the submission:

image image

Perhaps download it as json and inspect it before submitting it as it is?

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.02%. Comparing base (ef00987) to head (9e3135f).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #115   +/-   ##
=======================================
  Coverage   95.02%   95.02%           
=======================================
  Files          11       11           
  Lines         462      462           
=======================================
  Hits          439      439           
  Misses         23       23           
Flag Coverage Δ
unittests 95.02% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@northwestwitch northwestwitch merged commit 2740bc1 into main Apr 24, 2024
7 checks passed
@dnil
Copy link

dnil commented Apr 24, 2024

Other errors I see in the submission:
...
Perhaps download it as json and inspect it before submitting it as it is?

I wonder how they got in there? Added before fixes?

@northwestwitch
Copy link
Member Author

I wonder how they got in there? Added before fixes?

The errors? Could have happened before or after. The check for what's included in the associated conditions is not that strict, even if it kind of pre-fills the field for you already (so if you stick to what's there you should be fine). I wonder if we should make it super strict instead.. Like if you add an HP term then it should have the format HP:0000356 and not HP:HP:0000356 or HP:somedisease and so on..

@dnil
Copy link

dnil commented Apr 24, 2024

I wonder how they got in there? Added before fixes?

The errors? Could have happened before or after. The check for what's included in the associated conditions is not that strict, even if it kind of pre-fills the field for you already (so if you stick to what's there you should be fine). I wonder if we should make it super strict instead.. Like if you add an HP term then it should have the format HP:0000356 and not HP:HP:0000356 or HP:somedisease and so on..

Ah, ofcourse - they are manual adds?! I was wondering how they ended up there, and was searching for the events. 😆
Not wanting to be biased here, but I did see a peculiar user gender trend in the malformatted entries. 😂 We can always explain again, but I feel that with the pretty clear comment above, it would be best that manually entries are checked against our existing db entries. If we should allow manual input at all there? The submissions are brittle anyway, with little room for manual this-should-work-if-I-just-type-something-and-wish-for-it flair.

@dnil
Copy link

dnil commented Apr 24, 2024

I'll clean up that submission, but lets fix.

@northwestwitch
Copy link
Member Author

I'll clean up that submission, but lets fix.

Agreed

@northwestwitch northwestwitch deleted the fix_multiple_conditions_parsing branch April 26, 2024 10:55
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.

ClinVar upload issues..
3 participants