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

Feature add cornerstone adapters #225

Merged

Conversation

wayfarer3130
Copy link
Contributor

This change adds cornerstone adapters for a number of missing versions:

  1. Cobb Angle
  2. Angle
  3. RectangleRoi
  4. FreehandRoi

Some of these previously had code with "todo" in it, but no actual functionality.

The change also makes additional items bi-direction, in specific, the finding and findingSite values are now used to generate description and location values in cornerstone.

@pieper
Copy link
Collaborator

pieper commented Oct 22, 2021

@wayfarer3130 how does the work here relate to #197 ? Are they complementary and consistent?

@wayfarer3130
Copy link
Contributor Author

@wayfarer3130 how does the work here relate to #197 ? Are they complementary and consistent?

They are mostly complementary - there are a small number of overlapping changes, although this change makes #197 smaller because it puts a bunch of the work for #197 into a single location where it can just be done consistently.

@wayfarer3130 wayfarer3130 force-pushed the feature-add-cornerstone-adapters branch from 204128c to 879775e Compare October 25, 2021 11:29
@wayfarer3130 wayfarer3130 mentioned this pull request Oct 25, 2021
@dclunie
Copy link

dclunie commented Oct 25, 2021

@wayfarer3130 @swederik @JamesAPetts @igoroctaviano @galelis I noticed that the SR measurements code is checking CodeValue without checking CodingSchemeDesignator (e.g., contentSequenceArr.find(group => group.ConceptNameCodeSequence.CodeValue === FINDING) where FINDING_SITE = "G-C0E3".

This is bad practice - one should always check both value and scheme, since there is no guarantee that the same code will not be used in different schemes for different concepts (and indeed one should check for SRT, SNM3 and 99SDM if you want to be really robust).

Further, you should be using SCT not SRT codes on writing (but checking for either on reading for legacy support), e.g., SCT: 363698007 should be written rather than SCT: G-C0E3, in order to be compliant with CP 1850 and to keep the SNOMED licensing folks happy.

@wayfarer3130
Copy link
Contributor Author

Thanks @dclunie ,
I've made the changes to use the SCT codes rather than the SRT codes, and also to match on both coding scheme designator and code value instead of just code value. The old SRT codes are also accepted.

@wayfarer3130
Copy link
Contributor Author

Accidentally hit close instead of comment

@wayfarer3130 wayfarer3130 force-pushed the feature-add-cornerstone-adapters branch from 9827214 to b1da082 Compare October 25, 2021 20:20
@igoroctaviano
Copy link
Collaborator

@pieper tested against OHIF, LGTM.

@pieper pieper merged commit c65b930 into dcmjs-org:master Oct 26, 2021
@ohif-bot
Copy link
Collaborator

🎉 This PR is included in version 0.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants