Skip to content
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

Now possible to add existing label from an external file in the create-viewer option #2520

Merged
merged 20 commits into from
Nov 28, 2019

Conversation

lrouhier
Copy link
Contributor

@lrouhier lrouhier commented Nov 11, 2019

The goal of this PR is to create an option to see previously existing labels when using the -create-viewer option in sct_label_utils. The user could then add some missing labels or modify the existing one within the GUI.
DONE :

  • Create a -ilabel option allowing to enter the existing label files and extract the existing label coordinates and value

  • Modify the existing SagittalController in gui/sagittal.py to create the existing points on the GUI when showing the image.

  • When the user fails to enter enough label. example: sct_label_utils -i t2.nii.gz -ilabel labels.nii.gz -create-viewer 1,2 will create the GUI with all pre-existing labels as discussed in this PR.

  • Modify the code to project the existing point onto the mid_saggital plane.

@lrouhier lrouhier changed the title (WIP) sct_label_utils : add existing albel in the create-viewer option (WIP) sct_label_utils : add existing label in the create-viewer option Nov 11, 2019
@@ -20,8 +20,12 @@


class SagittalController(base.BaseController):
def __init__(self, image, params, init_values=None):
def __init__(self, image, params, init_values=None,previous_point=None):
Copy link
Member

@jcohenadad jcohenadad Nov 11, 2019

Choose a reason for hiding this comment

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

could you please add a space after "," (PEP8)

p.s. the same comment applies to all your other edits in this PR

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 will do.

@@ -69,6 +69,14 @@ def __init__(self, fname_label, fname_output=None, fname_ref=None, cross_radius=
self.fname_output = fname_output
else:
self.fname_output = fname_output

if isinstance(fname_previous, list):
Copy link
Member

Choose a reason for hiding this comment

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

why the need for this if? how about simply enforcing fname_previous to be a file name (could be verified by the parser)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right it will be enough. I just modeled it after the fname_output to begin with.

points=np.append(previous_points[i], np.array([previous_lab.data[previous_points[i][0], previous_points[i][1], previous_points[i][2]]]), axis=-1)
points=np.reshape(points, (1, 4))
previous_label=np.append(previous_label, points, axis=0)

Copy link
Member

Choose a reason for hiding this comment

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

no need for these extra lines

@lrouhier lrouhier changed the title (WIP) sct_label_utils : add existing label in the create-viewer option sct_label_utils : add existing label in the create-viewer option Nov 20, 2019
@lrouhier
Copy link
Contributor Author

update on the decision: when the user fails to enter all the labels already existing in the -ilabel argument files, it will just show the ones he asked for.
Existing points are projected onto the mid-sagittal plane as this is what shows up in the GUI, it is easier for the user to correct the label.

@jcohenadad
Copy link
Member

update on the decision: when the user fails to enter all the labels already existing in the -ilabel argument files, it will just show the ones he asked for.

sounds good (if i understand correctly)

Existing points are projected onto the mid-sagittal plane as this is what shows up in the GUI, it is easier for the user to correct the label.

that's the part i don't understand: if the gui only show the labels he asked for, how can he correct for the one he didn't ask for?

@lrouhier
Copy link
Contributor Author

lrouhier commented Nov 20, 2019

the -ilabel flag launch an algorithm that retrieves the existing coordinates. The retrieved coordinates are then passed as input for the GUI which considers them the same way it does selected points. The checklist on the right allows the modification of the coordinates of each label (retrieved from another file or clicked on). The user can correct the labels he did ask for if one is misplaced.

The drawback is that the points that are not asked for won’t be part of the new label image (for now). I was thinking that it was more coherent to just save what the user sees

@jcohenadad
Copy link
Member

the -ilabel flag launch an algorithm that retrieves the existing coordinates. The retrieved coordinates are then passed as input for the GUI which considers them the same way it does selected points. The checklist on the right allows the modification of the coordinates of each label (retrieved from another file or clicked on). The user can correct the labels he did ask for if one is misplaced.

The drawback is that the points that are not asked for won’t be part of the new label image (for now). I was thinking that it was more coherent to just save what the user sees

sorry i still don't understand-- let's me write a scenario to clarify:

  • users runs: sct_label_utils -i bla -create-viewer 3,4,5,6 -ilabel FILE_WITH_LABELS_2-3-4
  • GUI shows:
    • on the left (not on the right): the list: 2,3,4,5,6
    • on the image: points: 2,3,4
  • User can select/unselect whatever he wants. He can correct labels 2,3,4 if he wants
  • When clicking on save, labels 2,3,4,5,6 are saved

is that what you suggested?

if not, please explain with a step-by-step scenario for clarity-- thanks

@lrouhier
Copy link
Contributor Author

Not entirely. the scenario is the following:

  • user run sct_label_utils -i bla -create-viewer 3,4,5,6 -ilabel FILE_WITH_LABELS_2-3-4
  • GUI shows :
    • on the left : 3,4,5,6
    • on the image : points 3,4 (common points between what is already defined and what he asked for)
  • he can select what he wants and correct 3,4 if he wants to.
  • when clicking, on save labels 3,4,5,6 are saved

@jcohenadad
Copy link
Member

Not entirely. the scenario is the following:

  • user run sct_label_utils -i bla -create-viewer 3,4,5,6 -ilabel FILE_WITH_LABELS_2-3-4

  • GUI shows :

    • on the left : 3,4,5,6
    • on the image : points 3,4 (common points between what is already defined and what he asked for)
  • he can select what he wants and correct 3,4 if he wants to.

  • when clicking, on save labels 3,4,5,6 are saved

ok got it! how about adding 5,6 on the right, to give the user the possibility to correct for them? what are the cons?

@lrouhier
Copy link
Contributor Author

ok got it! how about adding 5,6 on the right, to give the user the possibility to correct for them? what are the cons?

Do you mean 2? ( I assumed FILE_WITH_LABELS_2-3-4 contained only label 2, 3 and 4 ).
The user can already add 5,6 on the image (they are not defined yet).

@jcohenadad
Copy link
Member

ok got it! how about adding 5,6 on the right, to give the user the possibility to correct for them? what are the cons?

Do you mean 2? ( I assumed FILE_WITH_LABELS_2-3-4 contained only label 2, 3 and 4 ).
The user can already add 5,6 on the image (they are not defined yet).

yes i meant 2, sorry

@lrouhier
Copy link
Contributor Author

I don't see any cons to it right now. I'll implement it and report if I find any problem with it.

@jcohenadad jcohenadad changed the title sct_label_utils : add existing label in the create-viewer option Now possible to add existing label from an external file in the create-viewer option Nov 26, 2019
@jcohenadad jcohenadad added this to the 4.1.1 milestone Nov 26, 2019
@jcohenadad jcohenadad added feature category: new functionality sct_label_utils context: labels Nov 26, 2019
Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

before starting my review, i would like to clarify the following points that are listed under TODO in the body text of this PR:

Decide what to do when the user fail to enter enough label. example : sct_label_utils -i t2.nii.gz -ilabel labels.nii.gz -create-viewer 1,2 will give an error if labels.nii.gz contains already more than two points. Should we raise an error before or increase the value argument to cover at least the number of existing point ?

I would suggest to increase the value argument to cover at least the number of existing point.

Modify the code to project the existing point onto the mid_saggital plane.

wasn't it implemented in commit efb4b71?

@lrouhier
Copy link
Contributor Author

I will modify the TO DO right now. Both of these are now implemented.

@@ -725,6 +762,11 @@ def get_parser():
mandatory=False,
example="t2_labels_cross.nii.gz",
default_value="labels.nii.gz")
parser.add_option(name="-ilabel",
Copy link
Member

Choose a reason for hiding this comment

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

move this option right below -create-viewer. It will be more intuitive

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

looks good!
there is just one thing to change, as it is not intuitive: if the user inputs a file with, let,s say, labels 2,3, and he is asking to label 4,5,6, once the window pops up, if the user clicks on the window, the label 2 will be changed (whereas it is already present). I would indicate in a text, that the user has to manually click on the check box on the left the label he would like to create (in that case, the user would click on 4). The text could be placed right above the labels on the left column.

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

👍

@jcohenadad jcohenadad merged commit 646f83f into master Nov 28, 2019
@jcohenadad jcohenadad deleted the lrouhier/sct_utils_guiwexisting_point branch November 28, 2019 19:47
jcohenadad pushed a commit that referenced this pull request Dec 18, 2019
…e-viewer option (#2520)

* first commit : Pints are added but are weirdly modified by 90 degree at each opening.WIP

* Fixed orientation Issue. Will open PR to discuss option about possible value or error.

*  correction after first review. All previous point are now saved in the mid_slice (if not modified)

* First version to remove the existing but not asked label. Trying to improve on the version.

* more elegant solution

* Added projection of existing point onto mid sagittal plane. Remove debugging ellemnt previously created

* typo and PEP8

* remove empty lines

* remove extra lines.

* Code modified to show every previous label found as well as asked for label. refactoring to use sct function.

*  PEP-8 modification

* PEP-8 formatting

* removing extra line. Modifying help description

* formatting

* moved the option following review. Added a warning message displayed only when using the -ilabel option as requested


Former-commit-id: 646f83f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature category: new functionality sct_label_utils context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants