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

feat: 🎸 DICOM SR STOW on MeasurementAPI #954

Merged
merged 59 commits into from
Dec 11, 2019
Merged

feat: 🎸 DICOM SR STOW on MeasurementAPI #954

merged 59 commits into from
Dec 11, 2019

Conversation

galelis
Copy link
Contributor

@galelis galelis commented Sep 26, 2019

User cases

As an user:

  • I should be able to add measurements and store as DICOM Structured Report
  • I should see a visual feedback after saving with success
  • I should see a visual feedback after saving with error (e.g. saving without any measurement)
  • I should be able to retrieve saved measurements from DICOM Structured Report
  • I should be able to see the retrieved measurements into the measurement table
  • I should be able to see the retrieved measurements drawn into the viewport
  • I should be able to see warnings on measurements of tools that are not supported by DICOMSR
  • I should be able to refresh the page and get all measurements saved restored
  • If I am not using a server type DICOMWeb, I shoulod not be able to see the save button neither retrieve measurements saved on DICOM Structured Report

Limitations

  • As an user, I can only save/restore measurements of LenghTool as a limitation on DCMJS, all other tools are going to get a warning into the measurement table
  • As an user, I can not save an empty list of measurements, as a limitation on DCMJS, I should see a error feedback
  • As an user, I can only use this feature if my server is a DICOMWeb server.

Screenshot

OHIF Viewer (1)

Closes: #758

@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #954 into master will decrease coverage by 0.22%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#core 14.11% <0%> (-0.02%) ⬇️
#viewer 0.38% <0%> (-0.03%) ⬇️
Impacted Files Coverage Δ
...OMSR/utils/findInstanceMetadataBySopInstanceUid.js 0% <0%> (ø)
platform/viewer/src/lib/DICOMSR/index.js 0% <0%> (ø)
...src/connectedComponents/ViewerRetrieveStudyData.js 0% <0%> (ø) ⬆️
...m/viewer/src/lib/DICOMSR/handleStructuredReport.js 0% <0%> (ø)
...ewer/src/lib/DICOMSR/parseDicomStructuredReport.js 0% <0%> (ø)
.../viewer/src/connectedComponents/ConnectedViewer.js 0% <0%> (ø) ⬆️
...rm/viewer/src/lib/DICOMSR/parseMeasurementsData.js 0% <0%> (ø)
...iewer/src/appExtensions/MeasurementsPanel/index.js 0% <0%> (ø) ⬆️
...rm/core/src/measurements/classes/MeasurementApi.js 0.23% <0%> (-0.01%) ⬇️
...ib/DICOMSR/utils/findMostRecentStructuredReport.js 0% <0%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cda515...825cc50. Read the comment docs.


import { api } from 'dicomweb-client';

const retrieveMeasurementFromSR = async series => {
Copy link
Member

@swederik swederik Oct 1, 2019

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.

@tiimoS
Copy link

tiimoS commented Oct 6, 2019

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

1. Deleting the last remaining measurement causes errors
There seem to be some problems when deleting the only remaining measurement using the context menu (right mouse click on measurement).

2. Not all measurement types are being supported (by dcmjs library)
Furthermore, as already mentioned by #988 it does not support saving measurements such as ArrowAnnotate or angle but throws an error:
Unhandled Promise Rejection: Error: TypeError: undefined is not an object (evaluating 'toolClass.getTID300RepresentationArguments')
This is mainly due to the fact that the dcmjs library only supports Length, Polyline and Bidirectional. Is there a way to edit the dcmjs library to also accept other types of measurements. I would like to prevent adding a database to store some measurements there, and others using the SR documents. It would be much better to deal with them in a unified manner.

3. Seems to generate a new SR for each measurement instead of one SR for all measurements
During my tests I have added quite some measurements. Each measurement added seems to have generated a separate SR document. These are not being deleted when you remove the measurements. Hence, I end up with a series consisting of about 200 SR documents (which I cannot delete in the client itself).

Currently, point 2. seems to the be the most important issue to be addressed. I would be happy to support you.

@dannyrb
Copy link
Member

dannyrb commented Oct 7, 2019

@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 dcmjs to support other tools. @JamesAPetts would probably be the best person to weigh in at this time.

@JamesAPetts
Copy link
Member

@JamesAPetts would probably be the best person to weigh in at this time.

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.

I would be happy to support you.

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!

const configuration = {
...measurementApiDefaultConfig,
};
const configuration = measurementApiDefaultConfig;
Copy link
Member

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';
Copy link
Member

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?

Copy link
Member

@dannyrb dannyrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@dannyrb dannyrb left a 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 👍

@dannyrb
Copy link
Member

dannyrb commented Dec 11, 2019

Most CI is failing here:

  1. OHIF Study Viewer Page "before all" hook for "checks if series thumbnails are being displayed":
    Uncaught ReferenceError: withSnackbar is not defined

We removed the import, but not it's usage:

const connectedComponent = withSnackbar(
withTranslation(['MeasurementTable', 'Common'])(MeasurementTable)
);

@sctkndl
Copy link

sctkndl commented Apr 1, 2020

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?

@pieper
Copy link
Member

pieper commented Apr 1, 2020

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.

https://docs.google.com/document/d/1bR6m7foTCzofoZKeIRN5YreBrkjgMcBfNA7r9wXEGR4/edit#heading=h.fgs65rsvrdy3

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

@galelis
Copy link
Contributor Author

galelis commented Apr 1, 2020

@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:
cornerstoneTools.globalImageIdSpecificToolStateManager.toolState;
It will return all measurements grouped by ImageId and toolName.

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:
https://github.com/OHIF/Viewers/blob/v1.x/Packages/ohif-measurements/client/reports/reportMeasurementData.js

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.

functionality Save to Archive
8 participants