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

Implement 'Detect and Select' component (#148) #226

Merged
merged 5 commits into from Nov 23, 2017

Conversation

Projects
None yet
4 participants
@Serhiy-Shekhovtsov
Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 15, 2017

Implemented draggable marker for Detect and Select page for issue #148

Description

vue-draggable-resizable component used to implement draggable nodule marker.
The marker is implemented as a separate component (open-image\NoduleMarker.vue) it takes marker (object with coordinates) param and navigation\zoom params zoomRate, offsetX, offsetY

Screenshots (if appropriate):

Detect and Select

What is not done:

  • loading image from backend(it requires a method for loading currently selected case)
  • saving mark\dismiss\coordinates on backend

If someone familiar with backend side would like to jump in and help to complete this page - I am more then happy to help from client side.

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
@@ -103,7 +103,6 @@
noduleCentroid.x = x
noduleCentroid.y = y
noduleCentroid.z = z
console.log(`coordinates saved, updating client model`, x, y)

This comment has been minimized.

@lamby

lamby Nov 15, 2017

Contributor

Were these added in this PR or elsewhere? If the former, why not remove them from your commits? :)

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 16, 2017

Contributor

I have removed this commit and updated two commits that introduced these changes. I didn't see a nice and easy way to do it yesterday night, but this morning I got an idea :)

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:origin/features/detect-and-select branch from 189d4c3 to 0e0cad8 Nov 16, 2017

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 17, 2017

LGTM, but would love some further eyes on this frontend stuff! @reubano ? :)

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 19, 2017

@lamby, @reubano is there anything blocking the merge of this PR?

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 19, 2017

Not on my end, just would like someone with some more Vue-foo than me to give it a once-over...

</tbody>
</table><br>
Centroid (predicted):<br>
<table style="margin-left:10px">

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

Moving style into style block.

@louisgv
Copy link
Contributor

louisgv left a comment

Some minor improvement suggestion

}
var noduleCentroid = this.selectedCandidate.centroid
return this.selectedCandidate ? {x: noduleCentroid.x, y: noduleCentroid.y, z: noduleCentroid.z} : null

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

This check is redundant since the condition has already been checked above. If anything, it should check for

noduleCentroid ? { etc...} : null;

Also should prefer and lean toward const over let and var. let only when mutability is needed which in this case is not.
Also, since selectedCandidate.centroid has all the property needed for the return, I think the code can be simplified to just:

marker() {
return this.selectedCandidate 
    ? this.selectedCandidate.centroid || null 
    : null
}

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 20, 2017

Contributor

First comment - for sure, just a mistake. Second thing - will fix. Last one - I am creating new object by intention in order to keep candidate's original coordinates until user finished dragging and they are saved on server. What do you think?

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

@Serhiy-Shekhovtsov Oh, that's a good point. If that's the case, you might want to do something like this:

if (!this.selectedCandidate || !this.selectedCandidate.centroid) return null

// Extract needed data and make sure they are immutable
const {x, y, z} = this.selectedCandidate.centroid

// Do stuff with data if needed
// ... write to localstore or server?

return {x,y,z}
return this.selectedCandidate ? {x: noduleCentroid.x, y: noduleCentroid.y, z: noduleCentroid.z} : null
},
selectedCandidate () {
return this.candidates[this.selectedCandidateIndex] ? this.candidates[this.selectedCandidateIndex] : null

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

This check can be simplified like so:

return this.candidates[this.selectedCandidateIndex] || null
// send data to backend
this.$axios
// .put('/api/candidates/1.json', {
.get('/api/candidates.json', {

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

Hmm.. is this correct?

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 20, 2017

Contributor

Since I have fixed the issue #231, I can fix this and also implement saving on the backend.

@@ -1,5 +1,7 @@
<template>
<div class="offset-top">

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

The class offset-top is local for /AnnotateAndSegment.vue and /ReportAndExport at the moment. Refer to this discussion: #173 (comment)

We might move offset-top up as a project-class? @lamby

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 20, 2017

Contributor

Actually, I added it to App.vue:
https://github.com/concept-to-clinic/concept-to-clinic/pull/226/files#diff-99d7a1bed87dbda2f915d10103d13394R28
But probably, it's a good idea to remove the copy\paste from other views.

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 20, 2017

Thanks for reviewing @louisgv! Will push updates today.

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:origin/features/detect-and-select branch from 0e0cad8 to 58fe68b Nov 20, 2017

.post(selectedCandidate.url + 'move', {
x: x,
y: y,
z: z

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

This can be simplified with ES6:

 .post(selectedCandidate.url + 'move', {x, y, z }

JS will grab the variable by name and apply its value as a key-value pair by itself :p

updated Candidate component to show only nudule details and Candidate…
…List to handle mark, dismiss, loading and saving marker

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:origin/features/detect-and-select branch from 58fe68b to e0f92c6 Nov 20, 2017

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 20, 2017

Requested changes have been implemented.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Nov 20, 2017

Looked over the python part and it seems to be ok. I have 0 vue experience tho so can't really comment on those files. Going to try it out in docker now to make sure the interface looks fine.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Nov 20, 2017

interface_1   | /usr/local/lib/python3.6/dist-packages/environ/environ.py:618: UserWarning: Error reading /app/.env - if you're not configuring your environment separately, check this.
interface_1   |   "environment separately, check this." % env_file)
interface_1   | Traceback (most recent call last):
interface_1   |   File "manage.py", line 32, in <module>
interface_1   |     execute_from_command_line(sys.argv)
interface_1   |   File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 363, in execute_from_command_line
interface_1   |     utility.execute()
interface_1   |   File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 307, in execute
interface_1   |     settings.INSTALLED_APPS
interface_1   |   File "/usr/local/lib/python3.6/dist-packages/django/conf/__init__.py", line 56, in __getattr__
interface_1   |     self._setup(name)
interface_1   |   File "/usr/local/lib/python3.6/dist-packages/django/conf/__init__.py", line 41, in _setup
interface_1   |     self._wrapped = Settings(settings_module)
interface_1   |   File "/usr/local/lib/python3.6/dist-packages/django/conf/__init__.py", line 110, in __init__
interface_1   |     mod = importlib.import_module(self.SETTINGS_MODULE)
interface_1   |   File "/usr/lib/python3.6/importlib/__init__.py", line 126, in import_module
interface_1   |     return _bootstrap._gcd_import(name[level:], package, level)
interface_1   |   File "<frozen importlib._bootstrap>", line 994, in _gcd_import
interface_1   |   File "<frozen importlib._bootstrap>", line 971, in _find_and_load
interface_1   |   File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
interface_1   |   File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
interface_1   |   File "<frozen importlib._bootstrap_external>", line 678, in exec_module
interface_1   |   File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
interface_1   |   File "/app/config/settings/local.py", line 5, in <module>
interface_1   |     from config.settings.base import *  # noqa
interface_1   |   File "/app/config/settings/base.py", line 133, in <module>
interface_1   |     APP_VERSION_NUMBER = f.readlines()[-1].split(' ')[1][:7]
interface_1   |   File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode
interface_1   |     return codecs.ascii_decode(input, self.errors)[0]
interface_1   | UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 3592: ordinal not in range(128)
alcf_interface_1 exited with code 1
@louisgv
Copy link
Contributor

louisgv left a comment

Front-end wise, LGTM 👍

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 21, 2017

@reubano I can't reproduce the issue you mentioned. Also the Travis build passed. Can you check it again, please?

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Nov 21, 2017

@Serhiy-Shekhovtsov ok, don't worry about it then.

@vessemer vessemer referenced this pull request Nov 21, 2017

Closed

Vue: implement 'Segmentation' UI component(s) #150

0 of 7 tasks complete
@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Nov 22, 2017

@lamby is everything good to go?

@lamby lamby merged commit 093b06f into drivendataorg:master Nov 23, 2017

2 checks passed

concept-to-clinic/cla @Serhiy-Shekhovtsov 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 Nov 23, 2017

Merged... Many thanks all!

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