Skip to content
This repository has been archived by the owner on Feb 22, 2020. It is now read-only.

#35 Show list of candidates nodules #88

Merged
merged 1 commit into from Sep 6, 2017

Conversation

WGierke
Copy link
Contributor

@WGierke WGierke commented Sep 3, 2017

This should add an "accordion" view that shows the candidate nodules for a selected image. The candidate fields lidc_max_sensitiv, convnet_vgg, convnett_vgg_lidc, Slice still need to be implemented in the back-end.

Description

I added the front-end Vue.js implementation to display candidate nodules of an image based on the current API.

Reference to official issue

This addresses #35 .

Motivation and Context

The doctor should be able to quickly get an overview of candidate nodules including their probability of being cancer.

How Has This Been Tested?

I created two dummy candidates by letting the API return

[{		
    "centroid": {		
        "x": 54,		
        "y": 78,		
        "z": 23,		
        "series": 0		
    },		
    "probability_concerning": 0.94,		
    "case": 1		
},		
{		
   "centroid": {		
       "x": 1,		
       "y": 2,		
       "z": 3,		
       "series": 1		
    },		
    "probability_concerning": 0.94,		
    "case": 1		
}]

The outcome was
screenshot from 2017-09-03 01-00-39
screenshot from 2017-09-03 01-00-55

CLA

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

@@ -3,3 +3,5 @@
padding: 2px;
width: auto;
}

.top-buffer { margin-top:20px; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should follow the existing style to some degree?

Copy link
Contributor Author

@WGierke WGierke Sep 4, 2017

Choose a reason for hiding this comment

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

I'm not sure what you mean? Currently, the main content of a page would be overlapped at the top to some degree by the navbar. Adding some margin to the content container in base.html solves that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Existing CSS code/indentation/spacing style, not the final result.. sorry for the confusion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see! Yeah, that would be way better :)

@@ -4,7 +4,7 @@

class SmokeTest(TestCase):
def test_landing(self):
url = reverse('static:home')
url = reverse('static:open_image')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think elsewhere we are using hyphens in url name attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the remark. I replaced all underscores in url names by hyphens.

@@ -7,16 +7,109 @@
{% endblock %}

{% block content %}
{{ block.super }}
{% verbatim %}
Copy link
Contributor

Choose a reason for hiding this comment

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

{% verbatim %} always seems like a bad smell to me in Django. How come we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Vue.js template syntax collides with the Django template syntax. Django would e.g. try to render following statements like Candidate {{ index + 1 }} or {{ candidate.centroid.x }} by itself, although the variables are provided by the Vue.js template language.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Vue.js template syntax collides with the Django template syntax

Getcha. I was wondering if we could move the {% verbatim %} blocks to only surround the colliding javascript in that case? That would make it a bit clearer why, and implicitly so. Alternatively, we could move the JS to an external file. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'd also prefer moving the tags so that they surround the "problematic" syntax as tightly as possible.

@WGierke
Copy link
Contributor Author

WGierke commented Sep 5, 2017

@lamby Thanks again for your feedback! I implemented it and squashed the commits.

@lamby lamby merged commit 774e214 into drivendataorg:master Sep 6, 2017
@lamby
Copy link
Contributor

lamby commented Sep 6, 2017

Thanks @WGierke — lamby

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants