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

#179 DICOM image viewer #188

Merged
merged 7 commits into from Oct 30, 2017

Conversation

Projects
None yet
5 participants
@vessemer
Copy link
Contributor

vessemer commented Oct 26, 2017

The ability to show DICOM images is one of the most important parts for this project. There are existing libraries that provide a way to show DICOM images that can help with this task. It were two main candidates with proper library to choose from: ivmartel and chafey. The decision was made to use the cornerstone library for the reason that it flexible and can provide the project with ability to show images in an interactive way. It is also was much easier to use with VueJS than its competitor.

Two main difficulties were met during the work on this issue. The first one was the problem with pixelData serialization. It took some time before a good solution was found.

The second problem was to make library work correctly with VueJS. Choosing the most appropriate llibrary also refers to this problem. A lot of time was spent on testing and studying different libraries and browsing repositories before the appropriate solution was found.

Description

This is the implementation of DICOM image viewer that was made with the help of cornerstone and cornerstoneTools. The image is being taken by the part of the url that adresses it.

Reference to official issue

This address #179

Motivation and Context

There is a strong need in the way of showing DICOM imagery. It is impossible to make that project at least somehow usable by common people without DICOMs actually being shown. Furthermore, issues #148 and #150 depend on it.

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

@vessemer vessemer changed the title DICOM image viewer #179 DICOM image viewer Oct 26, 2017

@antonow
Copy link
Contributor

antonow left a comment

Hey @vessemer, great work! I was working on this myself, so I know how hard it is to get figured out. I have one suggestion: I think the dicom viewer should be its own component instead of being inside the OpenImage view. The reason is that it will need to be imported into both the AnnotateAndSegment and DetectAndSelect views in addition to the OpenImage view.

@louisgv
Copy link
Contributor

louisgv left a comment

I think pulling in JQuery for a very simple op might not be a good decision

</template>

<script>
import ImageSeries from '../components/ImageSeries'
var cornerstone = require('cornerstone-core')

This comment has been minimized.

@louisgv

louisgv Oct 26, 2017

Contributor

I suggest avoiding var.

This comment has been minimized.

@vessemer

vessemer Oct 27, 2017

Contributor

Unfortunately, it fails if only I try one of following:

import { cornerstone }  from 'cornerstone-core'
------------------------------------------------------------
import cornerstone  from 'cornerstone-core'
Object.definePrototype(Vue.prototype, '$cornerstone', { value: cornerstone })
------------------------------------------------------------
import cornerstone  from 'cornerstone-core'
Vue.prototype.$cornerstone = cornerstone 
------------------------------------------------------------
import cornerstone from 'cornerstone'

export default {
  install: function(Vue) {
    Object.defineProperty(Vue.prototype, '$cornerstone', { value: cornerstone })
  }
}
</template>

<script>
import ImageSeries from '../components/ImageSeries'
var cornerstone = require('cornerstone-core')
var jquery = require('jquery')

This comment has been minimized.

@louisgv

louisgv Oct 26, 2017

Contributor

I think usage of jquery should be avoided. According to the code bellow, it seems like it was used to implement a promise. We should be able to use the native browser promise API instead of importing entire JQuery

This comment has been minimized.

@vessemer

vessemer Oct 27, 2017

Contributor

done, was used as a first through, thank you for the review!

},
methods: {
fetchData (id) {
this.$http.get('/api/images/metadata?dicom_location=/images/' + id.slice(this.csName.length + 3))

This comment has been minimized.

@louisgv

louisgv Oct 26, 2017

Contributor

To align with the other component, it's best to use the axios API instead.

This comment has been minimized.

@vessemer

vessemer Oct 27, 2017

Contributor

Implemented here

<image-series></image-series>
<div class="container">
<div class="col-xs-6">
<div style="width:512px;height:512px;position:relative;display:inline-block;color:white;"

This comment has been minimized.

@louisgv

louisgv Oct 26, 2017

Contributor

Style should be moved to the style tag

@jjjmm

This comment has been minimized.

Copy link
Contributor

jjjmm commented Oct 26, 2017

Hey @vessemer. Could you please next time indicate that you are working on the issue. You and me were working on the same thing at the same time

@vessemer vessemer force-pushed the vessemer:179_DICOM_viewer branch 3 times, most recently from 5398288 to d9f3bcb Oct 27, 2017

@vessemer vessemer force-pushed the vessemer:179_DICOM_viewer branch from d9f3bcb to 79a145e Oct 27, 2017

<div class="DICOM-container">
<div class="DICOM" ref="DICOM"></div>
</div>
<p>{{ dicom.imageId}}</p>>

This comment has been minimized.

@lamby

lamby Oct 27, 2017

Contributor

spacing here...

This comment has been minimized.

@vessemer

vessemer Oct 27, 2017

Contributor

Not a really necessary line :)

rowPixelSpacing: 1,
sizeInBytes: 512 * 512 * 2
},
dicomUrl: 'LIDC://LIDC-IDRI-0001/1.3.6.1.4.1.14519.5.2.1.6279.6001.298806137288633453246975630178/1.3.6.1.4.1.14519.5.2.1.6279.6001.179049373636438705059720603192/-95.000000.dcm',

This comment has been minimized.

@lamby

lamby Oct 27, 2017

Contributor

Ooh, magic string!

This comment has been minimized.

@vessemer

vessemer Oct 27, 2017

Contributor

it was just for testing, forget to exclude

},
methods: {
fetchData (id) {
this.$axios.get('/api/images/metadata?dicom_location=/images/' + id.slice(this.csName.length + 3))

This comment has been minimized.

@lamby

lamby Oct 27, 2017

Contributor

Can we not let Django generate the URIs instead?

This comment has been minimized.

@vessemer

vessemer Oct 27, 2017

Contributor

Sorry, not sure that I understand it correctly: did you mean that this URI prefix should be computed by the Django and then provided to vue?

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Oct 27, 2017

Hey, @antonow, thanks, it looks much better now
I spent last sleepless nights working on this PR, the main difficulties were related with the cornerstoneTool, besides for it has highly unstable behaviour (tests coverage status is 4% only) it also crashes with each update of the dependent cornerstone-core. The latest versions sustainable for our setup are:

"cornerstone-core": " 0.13.0",
"cornerstone-tools": "0.9.1"

Relating events implementation: It seems that it's complicated to find an actual guide on how to use $on and $emit without using buses. (at least for versions described above) so that the decision was made to use the bus to handle events. Here is the current view of DICOM slice selection:
peek 2017-10-27 13-24

@vessemer vessemer force-pushed the vessemer:179_DICOM_viewer branch from 5d429e2 to b57fb30 Oct 27, 2017

return bufView
},
getPixelData () {
var pixelDataAsString = window.atob(this.dicom.base64data)

This comment has been minimized.

@louisgv

louisgv Oct 27, 2017

Contributor

Suggest avoid var in these instances as well. Recommendation is to try to use const as much as possible so that we can adhere to immutable functional programming. 😈

Tho, direct alternative for var which is scope-safe would be to use let 😁

This comment has been minimized.

@vessemer

vessemer Oct 27, 2017

Contributor

Oh, yeah, thank you for pointing it out :)

@louisgv
Copy link
Contributor

louisgv left a comment

LGTM 👍

<script>
import { EventBus } from '../../main.js'
const cornerstone = require('cornerstone-core')
const Q = require('q')

This comment has been minimized.

@louisgv

louisgv Oct 28, 2017

Contributor

q is not very well maintained these day and it is also very old. I would suggest either looking forward with the Promise API implemented within the browser itself: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

Or use BlueBird for the perf boost as well as the issue discussed in this article: https://alexn.org/blog/2017/10/11/javascript-promise-leaks-memory.html

Blue bird: http://bluebirdjs.com/docs/getting-started.html

This comment has been minimized.

@vessemer

vessemer Oct 29, 2017

Contributor

Good point, done by this.

name: 'open-dicom',
data () {
return {
dicom: {

This comment has been minimized.

@louisgv

louisgv Oct 28, 2017

Contributor

I just went through cornerstone's example and have a better understanding of the library. Since we are using vuejs, and since we want this component to be re-usable, it is crucial that we might want to construct this entire object upon rendering the image instead of having a mocked version of it here with some binding to other places to get data.

My suggested implementation looks something like this:

  • This vue comp will accept parameters from parent component, which is the path to the image
  • With that, we move dicom to computed and simply do all the fetch and calculation there. With this we might not need to watch at all
  • On the same line of thought, we can simply construct the dicom object everytime the path changed, and then apply it using cornerstone.

The above should reduce the intertwine between static data and computed data that we are having here.

This comment has been minimized.

@louisgv

louisgv Oct 28, 2017

Contributor

Refer to this for computed variable in vue: https://vuejs.org/v2/guide/computed.html

This comment has been minimized.

@vessemer

vessemer Oct 29, 2017

Contributor

I've designed data flow through watch in order to ensure that data has been uploaded by asynchronous axios.get, now for the same goal, I've rewritten it in a promise-based style (with a chain of promises). It also turned out that each click a new canvas instance was created, this unexpected behaviour is fixed now.

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Oct 28, 2017

@louisgv Thanks for the review!

vessemer added some commits Oct 29, 2017

})
},
display () {
return this.dicom.then((dicom) => {

This comment has been minimized.

@louisgv

louisgv Oct 29, 2017

Contributor

For fancy async/await (it's shipped on latest chromium), we can also do something like this:

async display () {
   const element = this.$refs.DICOM
   this.initCS(element)
          
   cornerstone.registerImageLoader(this.view.type, this.dicom)
         
    const image = await cornerstone.loadImage(dicom.imageId)

    cornerstone.displayImage(element, image) 
}

Probably need to experiment with it a lil bit more. But overall, LGTM!

This comment has been minimized.

@vessemer

vessemer Oct 29, 2017

Contributor

Thanks, I've read about it, and implement by this.

This comment has been minimized.

@louisgv

louisgv Oct 30, 2017

Contributor

Sweet 👌

},
computed: {
info () {
return this.$axios.get(this.view.prefixUrl + this.view.path)

This comment has been minimized.

@louisgv

louisgv Oct 29, 2017

Contributor

For reference, here's async/await syntax for this:

async info() {
     const response = await this.$axios.get(this.view.prefixUrl + this.view.path); 
     return response.data;
}

This comment has been minimized.

@vessemer

vessemer Oct 30, 2017

Contributor

Thank you, done here

@louisgv
Copy link
Contributor

louisgv left a comment

LGTM 👍

@lamby lamby merged commit 680a524 into drivendataorg:master Oct 30, 2017

2 checks passed

concept-to-clinic/cla @vessemer has signed the CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Oct 30, 2017

Awesome. Thanks guys!

@vessemer vessemer deleted the vessemer:179_DICOM_viewer branch Oct 30, 2017

@vessemer vessemer referenced this pull request Nov 19, 2017

Closed

#145 import image series #233

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment