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

#145 import image series #233

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@vessemer
Copy link
Contributor

vessemer commented Nov 19, 2017

Import image series implementation.

Reference to official issue

This addresses #145

Screenshots:

ezgif com-cro

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
@@ -80,6 +87,7 @@
import { EventBus } from '../../main.js'
import TreeView from './TreeView'
import OpenDicom from './OpenDICOM'
const dirname = require('path-dirname')

This comment has been minimized.

@louisgv

louisgv Nov 19, 2017

Contributor

Hmm, seems like this module cannot be used with import? I thought import dirname from 'path-dirname' would work.

This comment has been minimized.

@vessemer

vessemer Nov 19, 2017

Contributor

Well, yes, it works like you thought. I was confused by this this obstacle for the cornerstone and cornerstone-tools. Thanks for pointing that out!

@vessemer vessemer force-pushed the vessemer:145_image_series_register branch from d49288b to 06122d2 Nov 19, 2017

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 19, 2017

@vessemer, I think it's a good idea to let others know when you are working on an issue (like I did here) if we are choosing collaboration over competition. This way team members will not waste their time working on same tasks.

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 19, 2017

Image loading is also a part of this PR that also shows currently running case, allows user to import and start new case after preview image, as it was described in documentation.

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Nov 20, 2017

peek 2017-11-20 06-51

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 21, 2017

FYI I've started a discussion here #236 (comment) regarding the potential feature conflict..

@vessemer vessemer force-pushed the vessemer:145_image_series_register branch from baefcbc to 4c1420f Nov 23, 2017

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Nov 23, 2017

peek 2017-11-23 02-38



@api_view(['POST'])
def case_available(request):

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 24, 2017

Contributor

There is no need to create a new method. cases/ will do just fine.

class="btn btn-primary float-left case-btn">
Create new case
</button>
<a href="#/report-and-export" class="btn btn-primary float-left case-btn">Start case</a>

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 24, 2017

Contributor

As far as I understood from this comment we should have multiple cases running same time. So there is no need to have Start case button. At least until there is no any role for it :)

class="btn btn-primary float-left case-btn">
Create new case
</button>
<a href="#/report-and-export" class="btn btn-primary float-left case-btn">Start case</a>
</template>
<template v-else>
<p class="card-text">No images imported.</p>
</template>
<button class="btn btn-warning float-right"

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 24, 2017

Contributor

Please, remove this button(Show\Hide import) and make the tree view always visible, there is nothing else to show on this page. The only role of this page is to select a file and start the case, so there is no reason to require user to do extra clicks.

@@ -123,10 +136,28 @@
// TODO: handle error
})
},
async fetchExistedCases (series) {

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 24, 2017

Contributor

fetchExistedCases -> fetchExistingCases

@@ -141,6 +172,9 @@
</script>

<style lang="scss" scoped>
.case-btn {
margin-left: 1%

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 24, 2017

Contributor

Bootstrap has classes like ml-1, ml-2 etc. for margins. Check this.

@@ -141,6 +172,9 @@
</script>

<style lang="scss" scoped>
.case-btn {
margin-left: 1%
}
.left {
float: left;

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 24, 2017

Contributor

You can use Bootstrap class .pull-left for this.

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 24, 2017

One more thing, since you have fixed the DICOM viewer - user can scroll through slices now. So, clicking on last folder should actually show the image. No need to show list of files. You can use my code here.

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 24, 2017

Thanks for reviewing @Serhiy-Shekhovtsov :)

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 27, 2017

Hey @vessemer, I assume you got all these comments, etc.? :)

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 27, 2017

No rush of course but the build seems to be failing and, alas, we need to resolve these conflicts :)

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 28, 2017

Gentle ping on this? :)

@vessemer vessemer force-pushed the vessemer:145_image_series_register branch 2 times, most recently from 74ebeb6 to 7530822 Nov 29, 2017

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 29, 2017

@lamby, @reubano, I can reopen and update my PR if it will help us to proceed on this.

@vessemer vessemer force-pushed the vessemer:145_image_series_register branch from 7530822 to 79ecca7 Nov 29, 2017

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 1, 2017

@Serhiy-Shekhovtsov no worries. tests are now passing and conflicts have been fixed.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 1, 2017

Well, for whatever reason I'm not able to duplicate your screencasts. Here's what I get.

screen shot 2017-11-30 at 23 10 44
screen shot 2017-11-30 at 23 14 22

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Dec 1, 2017

@Serhiy-Shekhovtsov Thanks so much. However, I still see two remaining conflcts? :)

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Dec 1, 2017

Great job @vessemer! Few small comments from my side:

  • there are two buttons on the page: Create case and Start case and one of them does nothing but confuses the user :)
  • also I think we should not show the list of files under last folder. Because once we switch to real data - we'll see hundreds of files there. It will make it complicated for user to browse the tree and this list doesn't do any help, cause the DICOM viewer should already support scrolling through slices. Again, you can copy\merge this code.
@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 1, 2017

Turns out I forgot the --hard option in my git reset command. I'm getting the changed code now. Is there a data import step I need to run first?

I'm getting this:

vue_1            |  DONE  Compiled successfully in 25960ms18:58:35
vue_1            | 
vue_1            | > Listening at http://0.0.0.0:8080
vue_1            | 
vue_1            | [HPM] Error occurred while trying to proxy request /api/images/ from 52.207.235.82:8080 to http://interface:8000 (ENOTFOUND) (https://nodejs.org/api/errors.html#errors_common_system_errors)
@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Dec 5, 2017

@vessemer Ping on this? :)

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 5, 2017

Anyone else seeing an [HPM] Error? I'm still getting it. Even on master.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 5, 2017

Re-ran everything and it's looking good now!

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 5, 2017

@Serhiy-Shekhovtsov great suggestions above. I've just fixed the conflicts so this PR should be merged soon. Mind submitting issues and accompanying PRs of your ideas?

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 5, 2017

Great job @vessemer! Once the tests complete (and hopefully pass!) do you mind rebasing everything to 1 commit? Thanks!

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Dec 5, 2017

We should not show the list of files under last folder. Because once we switch to real data - we'll see hundreds of files there. It will make it complicated for user to browse the tree and this list doesn't do any help, cause the DICOM viewer should already support scrolling through slices.

This is still not fixed, right?

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 5, 2017

@vessemer vessemer force-pushed the vessemer:145_image_series_register branch from 3611f1c to 49ce1ac Dec 5, 2017

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 5, 2017

There's a candidate_dismiss is not defined error in interface/backend/api/urls.py.

import image series
Case manager

Handy case selection

some fixes

@vessemer vessemer force-pushed the vessemer:145_image_series_register branch from 49ce1ac to cc82522 Dec 5, 2017

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Dec 5, 2017

FYI, @pjbull and I are in the midst of a cleanup and refactor right now that will take these changes into account - we can mark this is semi-accepted (thanks @vessemer and @Serhiy-Shekhovtsov for all the footwork). We'll credit appropriately.

[cc @reubano]

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Dec 9, 2017

@isms Don't forget to re-ping here once done so we can resolve the conflicts :)

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Dec 11, 2017

@lamby All set from my perspective - we can close this and acknowledge contributors with full points. (This PR was really helpful in framing how we wanted things to work here, it just ended up being too far from the major refactored version to merge as-is. It otherwise would have been merged 👍)

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Dec 12, 2017

@isms Done! Many thanks all o/

@lamby lamby closed this Dec 12, 2017

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