drivendataorg / concept-to-clinic

ALCF Concept to Clinic Challenge
https://concepttoclinic.drivendata.org/
MIT License
367 stars 147 forks source link

#145 import image series #233

Closed vessemer closed 6 years ago

vessemer commented 7 years ago

Import image series implementation.

Reference to official issue

This addresses #145

Screenshots:

ezgif com-cro

CLA

Serhiy-Shekhovtsov commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

peek 2017-11-20 06-51

lamby commented 7 years ago

FYI I've started a discussion here https://github.com/concept-to-clinic/concept-to-clinic/pull/236#issuecomment-345875065 regarding the potential feature conflict..

vessemer commented 7 years ago

peek 2017-11-23 02-38

Serhiy-Shekhovtsov commented 7 years ago

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 commented 7 years ago

Thanks for reviewing @Serhiy-Shekhovtsov :)

lamby commented 6 years ago

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

lamby commented 6 years ago

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

lamby commented 6 years ago

Gentle ping on this? :)

Serhiy-Shekhovtsov commented 6 years ago

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

reubano commented 6 years ago

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

reubano commented 6 years ago

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 commented 6 years ago

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

Serhiy-Shekhovtsov commented 6 years ago

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

reubano commented 6 years ago

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 commented 6 years ago

@vessemer Ping on this? :)

reubano commented 6 years ago

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

reubano commented 6 years ago

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

reubano commented 6 years ago

@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 commented 6 years ago

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

Serhiy-Shekhovtsov commented 6 years ago

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 commented 6 years ago

@Serhiy-Shekhovtsov correct.

reubano commented 6 years ago

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

isms commented 6 years ago

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 commented 6 years ago

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

isms commented 6 years ago

@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 :+1:)

lamby commented 6 years ago

@isms Done! Many thanks all o/