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

Segments loaded from DICOM should have names as in DICOM SEG #119

Open
fedorov opened this issue Nov 26, 2016 · 14 comments
Open

Segments loaded from DICOM should have names as in DICOM SEG #119

fedorov opened this issue Nov 26, 2016 · 14 comments
Assignees
Labels

Comments

@fedorov
Copy link
Member

fedorov commented Nov 26, 2016

Currently segments names in the measurements table are correctly set from DICOM, but the segments list names are incorrect.

image

@fedorov fedorov added the bug label Nov 26, 2016
@che85
Copy link
Member

che85 commented Nov 26, 2016

I am not sure, how the Name column is populated. For the measurements table, I am calling segment.GetName()

@cpinter How are those names populated when loading them from DICOM?

@fedorov
Copy link
Member Author

fedorov commented Nov 26, 2016

Isn't there a setter for the name?!

@che85
Copy link
Member

che85 commented Nov 26, 2016

Yes there is, but it doesn't make sense to me that the names are displayed incorrectly. When you create a segmentation on your own and then change the segment name from the segment table, then it will also change in the measurements table. That's why I am wondering.

@cpinter
Copy link
Collaborator

cpinter commented Nov 26, 2016

In the DICOM SEG loader, the segment name is the same as the temporary labelmap volume's name that was generated. I believe those nrrds are written out by your CLI

@cpinter
Copy link
Collaborator

cpinter commented Nov 26, 2016

This is where segment name is set when loading:
https://github.com/QIICR/QuantitativeReporting/blob/master/Py/DICOMSegmentationPlugin.py#L278
I don't know where the proper name should come from, but it needs to be set here

@che85
Copy link
Member

che85 commented Nov 26, 2016

@cpinter Wouldn't it make sense to populate that from the terminology?

@che85
Copy link
Member

che85 commented Nov 26, 2016

But in any case, there is something wrong then. I will need to check .

@fedorov
Copy link
Member Author

fedorov commented Nov 26, 2016

I will need to look as well. As I think about it, there is a name for the measurements group in SR, and segment label in SEG. They don't have to be the same, and I think they are not. Please don't spend any time on this until I take a look. Sorry for the confusion!

@fedorov
Copy link
Member Author

fedorov commented Nov 26, 2016

In either case, there is a setter for segment name, right Csaba?

@cpinter
Copy link
Collaborator

cpinter commented Nov 26, 2016

@che85 I didn't make any decisions re segment name, I simply made it work that was implemented at the NA-MIC week by Kyle (if I remember correctly).

@fedorov segment->SetName(newName)

@cpinter
Copy link
Collaborator

cpinter commented Nov 26, 2016

Please see the line I referred to a few comments above. The argument of that call simply needs to be replaced from the proper name. I'm not sure what I can do to help further.

@fedorov
Copy link
Member Author

fedorov commented Nov 26, 2016

Csaba, I will look at it later today or tomorrow. Thanks for the clarification, nothing else is needed.

@fedorov
Copy link
Member Author

fedorov commented Nov 27, 2016

Ok, so there are several issues:

I think here's what we should do:

  • add an option to specify SegmentLabel to the SEG schema
  • save SegmentLabel into JSON file while converting from SEG
  • while loading segments, assign segment name to be SegmentLabel from JSON
  • while populating the measurements table, have one column in the table to be "Tracking Identifier" loaded from SR (it is in the schema), and another one for the SegmentLabel of the segment that was used to calculate the measurements in that measurements group

@fedorov
Copy link
Member Author

fedorov commented Nov 27, 2016

To avoid any confusion - none of this is Csaba's fault! :)

@fedorov fedorov self-assigned this Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants