-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: 🎸 DICOM SR STOW on MeasurementAPI #954
Conversation
Codecov Report
@@ Coverage Diff @@
## master #954 +/- ##
=========================================
- Coverage 10.05% 9.83% -0.23%
=========================================
Files 260 269 +9
Lines 6591 6739 +148
Branches 1244 1264 +20
=========================================
Hits 663 663
- Misses 4799 4927 +128
- Partials 1129 1149 +20
Continue to review full report at Codecov.
|
platform/viewer/src/appExtensions/MeasurementsPanel/ConnectedMeasurementTable.js
Outdated
Show resolved
Hide resolved
|
||
import { api } from 'dicomweb-client'; | ||
|
||
const retrieveMeasurementFromSR = async series => { |
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.
Can you talk to @ladeirarodolfo about using the loader service he added? That way this can also work for local files, and cases where WADO-RS is not available.
Generally seems to work. I observed three obstacles though:
1. Deleting the last remaining measurement causes errors 2. Not all measurement types are being supported (by dcmjs library) 3. Seems to generate a new SR for each measurement instead of one SR for all measurements Currently, point 2. seems to the be the most important issue to be addressed. I would be happy to support you. |
@tiimoS, number two is interesting. I'm sure we intend to eventually cover the tools from cornerstone tool's we include, but I'm not sure of the progress, or how to extend |
Its simply an issue of adding definitions for how we translate the current measurements to/from SR. The SR implementation in dcmjs is certainly not exhaustive yet, but as Danny says, we will make sure to cover the core tools in due course in dcmjs.
If you want to help the effort by working on support for any other cornerstone SR adapters in dcmjs I'll be happy to review a PR! |
platform/viewer/src/lib/DICOMSR/retrieveDataFromMeasurements.js
Outdated
Show resolved
Hide resolved
…the data is not Number (it comes as string from dcmjs)" This reverts commit 4b3a1ef.
const configuration = { | ||
...measurementApiDefaultConfig, | ||
}; | ||
const configuration = measurementApiDefaultConfig; |
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.
If we're not going to expand the config, spreading is safer. It prevents accidentally modifying the default config by reference.
@@ -10,6 +10,7 @@ import PropTypes from 'prop-types'; | |||
import { ScrollableArea } from './../../ScrollableArea/ScrollableArea.js'; | |||
import { TableList } from './../tableList'; | |||
import { Tooltip } from './../tooltip'; | |||
import { withSnackbar } from './../../contextProviders/SnackbarProvider'; |
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.
This is still pulling in withSnackbar
?
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.
Follow-up tickets:
https://github.com/OHIF/Viewers/pull/954/files#r354547871- Created here: Create a measurements service #1281
- https://github.com/OHIF/Viewers/pull/954/files#r354551681
- https://github.com/OHIF/Viewers/pull/954/files#r354553655
- https://github.com/OHIF/Viewers/pull/954/files#r354557627
- Handling annotation schema versioning, missing tools for supported adapters, and other hazards
Split and added as new features or fixes:
Blocking:
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.
LGTM on the code front 👍
Most CI is failing here:
We removed the import, but not it's usage: Viewers/platform/ui/src/components/measurementTable/MeasurementTable.js Lines 208 to 210 in 2bba0a2
|
I'm interested in testing out this MeasurementAPI, can someone point me to any details on how to use it, please? I'm interested in using it mostly with Bidirectional Tool, and probably not with a Dicomweb server (I'm thinking storing in json somewhere maybe). With some guidance on getting started, I may be able to design an alternative that works for all tools. Alternatively, I could look at dcmjs and expanding it's capability to store data from other tools, unless that's already been ruled out? |
Hi - The plan is definitely to expand dcmjs to handle all the measurement types. We plan to follow the pattern put forth in the white paper linked below. Any help on that would be much appreciated. It will be great once it's implemented but we also understand some one-off json is much more convenient sometimes. You might also be interested in this draft standard for simplifying the creation of SR documents: ftp://medical.nema.org/medical/dicom/supps/Frozen/sup219_fz_JSONSR.pdf |
@sctkndl Hi, Working on DCMJS and adding support for more tools would be fantastic and for that we have another issue created and a big discussion happening around it. #1215 If your issue is just to store measurements on OHIF, we use cornerstoneTools and you can access all measurements by doing: If you want to work with the measurementAPI together with the MeasurementTable, on OHIF v1.x we had an implementation that we used to export measurementAPI data into a CSV file. You can see the implementation here: |
User cases
As an user:
Limitations
Screenshot
Closes: #758