-
Notifications
You must be signed in to change notification settings - Fork 659
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
Updated snomed codes, based on the exchange discusssion with Dylan #1520
Conversation
@@ -158,15 +158,15 @@ | |||
"codes": [ | |||
{ | |||
"system": "SNOMED-CT", | |||
"code": "384758001", | |||
"code": 384758001, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice this before but there are some instances in the diff like this one, where neither the code nor display value changed, but the code was changed from a string to a number. Any idea what happened here? It doesn't seem like all codes were changed. This isn't a huge deal but if it's possible to not change the type if there wasn't a value change, or just make them all strings instead of numbers, that would be preferable to making them numbers
@dehall - I have noticed that as well when I was writing the script, some were numbers, and some were strings, so I thought it would be better to be consistent and when I was doing the check, changed them to numbers. I can make the adjustment and make them strings, not a problem at all. Should I do it? |
Yes, if that's a simple change please do that. Thanks! For more detail: All codes are really strings, in the case of SNOMED it's usually safe to represent them as numbers but not always, so better to keep them as strings. (And LOINC for example includes a dash so the codes are necessarily strings.) The reason many of them are stored as numbers now is our module builder UI only has one text entry component and it tries to infer the type from the content, so a string that looks numeric turns into a number. This has been a problem before: synthetichealth/module-builder#280 |
@dehall - perfect, I see. I did the changes and converted them into strings, and committed the changes. If there is any other change needed, please let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, and especially for your patience as we worked through this to get it merged!
@dehall - thank you as well for the discussions and the help to make this PR great. I will reach to you later when you are more free and we can discuss on the way to update the rest of the codes. Thank you one more time, and wish you a great day |
Pull Request Description: Updated SNOMED Codes
Overview
This pull request aims to update the SNOMED codes across various Synthea modules, ensuring they are current and active. The updates were made using a script specifically developed for this purpose, and from an exchange discussion with Dylan.
Changes Made
Script Development: A custom script was created to review and update the SNOMED codes within the Synthea modules. The script performed the following tasks:
Code Updates:
What are the benefits:
Remaining Issues
Future Work
Thank you for considering this pull request.