specifying meta-data for DICOM Segs in Quantitative Reporting module

[chribaue]

The exported DICOM Segs currently store proper information what the segments encode and from which dataset they were derived. However, there are still items that need to be done to properly encode who produced the segmentation for what purpose and with which tool/algorithm. Browsing through your code, I can see that during development of dcmqi and the Quantitative Reporting module you already planned ahead for supporting these items, but so far, several meta-data properties are only set to generic default values and there's no way for the user to specify them through the Slicer GUI.

https://qiicr.gitbooks.io/dcmqi-guide/user_guide/itkimage2segimage.html neatly lists all these properties that should be properly specified:

general content information:

• "ContentCreatorName"
• "ClinicalTrialSeriesID"
• "ClinicalTrialTimePointID"
• "ClinicalTrialCoordinatingCenterName"
• "SeriesDescription"
• "SeriesNumber"
• "InstanceNumber"

for each segment:

• "SegmentAlgorithmType"
• "SegmentAlgorithmName"

I guess for the "general information" a simple dialog box and a mechanism that stores the specified information between Slicer sessions will suffice.

Regarding information about algorithm type and name, I would say this information should be provided by the producing algorithm and I would propose that algorithms that want to provide this information should add proper information to the Segment's tags, i.e.:

• segment.SetTag('DICOM.SegmentAlgorithmName', "PET Tumor Segmentation v1.0")
• segment.SetTag('DICOM.SegmentAlgorithmType', "SEMIAUTOMATIC")

During exporting DICOM Segs this information would be used, if present, and default values would be used otherwise.

[che85]

Also when Loading a SR/SEG we need to make sure that the displayed information are consistent. "Reader" within the watchbox ideally would be ContentCreatorName taken from DICOM

[che85]

I think for now this could just be a QDialog taking as a parameter a json schema (including default values). On completion it could return a json string holding the filled out information or default information.

[chribaue]

@che85 For the "general information" it could be as simple as that. It would just be nice if this information (content creator name...) would get stored between sessions.

What about the Segmentation Algorithm? The user might create different segments with different algorithms.

[che85]

@chribaue I just thought about storing that information between sessions, which absolutely makes sense. I might add some module settings, where users can add such default information to be stored in qSettings.

Regarding the segmentation algorithm, we should maybe also involve @lassoan and @cpinter. Maybe they have a good idea how to address that.

[che85]

I implemented it like this.

The checkbox indicates, that the setting is saved as default in QSettings "QuantitativeReporting/GeneralContentInformationDefaults"

The input json to that widget looks like the following:

{
  "ContentCreatorName": {
    "type" : "string",
    "maxLength" : 64,
    "default": "Reader1"
  },
  "ClinicalTrialSeriesID": {
    "type" : "string",
    "maxLength" : 64,
    "default": "Session1"
  },
  "ClinicalTrialCoordinatingCenterName": {
    "type" : "string",
    "maxLength" : 64,
    "default": "dcmqi"
  },
  "ClinicalTrialTimePointID": {
    "type" : "string",
    "maxLength" : 64,
    "default": "1"
  },
  "SeriesDescription": {
    "type" : "string",
    "maxLength" : 64,
    "default": "Segmentation"
  },
  "SeriesNumber": {
    "type" : "string",
    "maxLength" : 12,
    "default": "300"
  },
  "InstanceNumber": {
    "type" : "string",
    "maxLength" : 12,
    "default": "1"
  }
}

TODO: integrate with QuantitativeReporting

[chribaue]

The dialog box looks good to me.

Regarding the json, I think the InstanceNumber and IDs shouldn't be strings but numbers. Also, the form fields should get validated to make sure the dialog box only returns information valid for the DICOM SR.

[che85]

@chribaue In DICOM instanceNumber is defined as IS which is a integer string. [1] ClinicalTrialTimePointID and ClinicalTrialTimePointID are DICOM value representation type LO, which is a long string [1]

We need to make sure to be DICOM compliant rather than what we assume it should be. But I agree that it can be quite confusing because of the namings given for DICOM tag values.

P.S. I will take care of validation.

[1] http://dicom.nema.org/medical/dicom/current/output/chtml/part05/sect_6.2.html#para_f9be0f37-12c8-4be9-aa3f-eb9a5ee10dd3

[fedorov]

I also think that is better to set SeriesNumber and InstanceNumber to be numbers in the dialog, and then cast to string at the time they are passed to dcmqi. I understand this can be unconstrained and then validated, but it can create unnecessary confusion. We can't constrain the input to the dialog text field to match DICOM IS VR!

[pieper]

We can't constrain the input to the dialog text field to match DICOM IS VR!

Why not? It seems simple enough, especially if we have the python json schema validator that was mentioned elsewhere.

[lassoan]

Yes, it's very easy to constrain Qt line edit using a regexp validator.

What does default checkbox mean? Save to default? It is unusual to have "save to default" for each item in a form. Usually it is just a single checkbox at the bottom (that saves on Accept) or a pushbutton (that saves immediately). When default setting is available then it is useful to have a "Reset to default" pushbutton, too.

Field values:

• "ContentCreatorName": Saving the operator name could be useful in a number of different scenarios (required for HIPAA compliance, etc), so at some point, it will be added to general settings (in Slicer.ini, in "General" section or something similar). To avoid redundancy and inconsistency in the future, it would be better to add this value in a general section already (and not in a not in a module-specific section, such as "QuantitativeReporting").
• "ClinicalTrialCoordinatingCenterName", "ClinicalTrialSeriesID", "ClinicalTrialTimePointID": these are module/workflow specific values, probably QuantitativeReporting section is good
• "SeriesDescription", "SeriesNumber", "InstanceNumber": These all should come from the source data (and stored in node attributes or subject hierarchy attributes) or generated automatically.
• "SegmentAlgorithmType": the generator module knows it, so probably there is no need to ask the user (and it would be probably always "SEMIAUTOMATIC").
• "SegmentAlgorithmName": There is no chance that any algorithm can be described in a few words, so I think something like "Segment Editor - Slicer 4.8.1" would work (if there is a seprate field for algorithm version then the version information should be moved there).

[fedorov]

We can't constrain the input to the dialog text field to match DICOM IS VR!

Why not? It seems simple enough, especially if we have the python json schema validator that was mentioned elsewhere.

Interesting. I wonder how that regex constraint would be communicated to the user typing the value. But in any case - can you explain why do you think this is needed? For all practical purposes, series and instance numbers are integer numbers, seems like an obvious solution to set this field to integer.

"ContentCreatorName": Saving the operator name could be useful in a number of different scenarios

I think the difference is that here it is required (or will be populated with default). I agree it would be nice to have it application-wide. I would even prefer it is initialized on first startup of the application. But it may confuse user who does not need it.

"SeriesDescription", "SeriesNumber", "InstanceNumber": These all should come from the source data

The objects created in QR will be new DICOM instances, and so ideally these numbers should be unique within the study.

"SegmentAlgorithmType": the generator module knows it, so probably there is no need to ask the user (and it would be probably always "SEMIAUTOMATIC").

Yes, and ideally it would be initialized by SegmentEditor keeping track about what effects were applied during segmentation.

"SegmentAlgorithmName": There is no chance that any algorithm can be described in a few words

When a single effect was used, it would be nice to assign this to the name of that effect. Specifically, in the case of the PET segmentation tool developed by @chribaue, it is important to capture that fact. Again, would be nice if this was taken care of automatically by SegmentEditor.

[pieper]

We can't constrain the input to the dialog text field to match DICOM IS VR!

Why not? It seems simple enough, especially if we have the python json schema validator that was mentioned elsewhere.

Interesting. I wonder how that regex constraint would be communicated to the user typing the value. But in any case - can you explain why do you think this is needed? For all practical purposes, series and instance numbers are integer numbers, seems like an obvious solution to set this field to integer.

It would be just like a validation field in a web page where they say that your age can't include a letter or your username can't include punctuation.

As a general UI rule it's best to validate the input while the user is focused on the task rather than giving a validation error (or worse) later.

[lassoan]

"ContentCreatorName": Saving the operator name could be useful in a number of different scenarios

I think the difference is that here it is required (or will be populated with default). I agree it would be nice to have it application-wide. I would even prefer it is initialized on first startup of the application. But it may confuse user who does not need it.

I would just suggest to store it in a generic section in the ini file and probably make it also editable in application settings. We should probably still display it in this popup window so that users can confirm that it is correct.

"SeriesDescription", "SeriesNumber", "InstanceNumber": These all should come from the source data

The objects created in QR will be new DICOM instances, and so ideally these numbers should be unique within the study.

Probably unique numbers should be generated automatically from source data and there is probably no need to ask the user to edit them (they can still be displayed in an advanced section so that the user can see them and maybe change them). It is probably not ideal to store the value directly in settings, but instead storing a minimum ID value or offset might be better (e.g., you could specify to start SeriesNumber from 900; then when you generate the unique ID then you would search available IDs starting from 900).

"SegmentAlgorithmType": the generator module knows it, so probably there is no need to ask the user (and it would be probably always "SEMIAUTOMATIC").

Yes, and ideally it would be initialized by SegmentEditor keeping track about what effects were applied during segmentation.

If you allow the user to freely use the Segment Editor then it is not useful to just store the list of effects that were used, because:

1. The segmentation process will not be reproducible anyway (unless we store for each applied effect all input parameters and cursor positions)
2. User should manually verify segmentation results, even if semi-automatic tools, such as threshold paint, region growing, or smoothing are used. So, the method will be eventually semi-automatic.

"SegmentAlgorithmName": There is no chance that any algorithm can be described in a few words

When a single effect was used

If it is feasible to store all segmentation inputs in the DICOM fields (for example, in case of fully automatic methods), then it would make sense to store all these inputs. But probably this is very rare and it means the Segment Editor is not used (as editing is inherently "semi-automatic" - use some automated tools with manual user input).

I think it would not be too difficult to implement storing of segmentation editing history in a string that could be written to DICOM tags, but I'm not sure that dumping that complex information into a few DICOM fields would be useful. No users or other software than Slicer could interpret that information.

[che85]

@fedorov

The objects created in QR will be new DICOM instances, and so ideally these numbers should be unique within the study.

Maybe we can do that by parsing the DICOMDatabase on master volume selection and deciding based on that

@pieper

As a general UI rule it's best to validate the input while the user is focused on the task rather than giving a validation error (or worse) later.

I absolutely agree with that!

@lassoan

What does default checkbox mean? Save to default? It is unusual to have "save to default" for each item in a form. Usually it is just a single checkbox at the bottom (that saves on Accept) or a pushbutton (that saves immediately). When default setting is available then it is useful to have a "Reset to default" pushbutton, too.

I added those default checkboxes for every single field, because the user might want to set partially as default. Additionally, if checked on initialization, the user will know, which fields are set as defaults in the QSettings.

"ContentCreatorName": Saving the operator name could be useful in a number of different scenarios

I think the difference is that here it is required (or will be populated with default). I agree it would be nice to have it application-wide. I would even prefer it is initialized on first startup of the application. But it may confuse user who does not need it.

I would just suggest to store it in a generic section in the ini file and probably make it also editable in application settings. We should probably still display it in this popup window so that users can confirm that it is correct.

At the moment defaults are either defined by the delivered json file or by QSettings (which can be another parameter during initialization). If QSettings are available, it will be prioritized over json file defaults.

Generally spoken, I think that ContentCreatorName should be something more meaningful, than just using getpass.getuser(). I think this should be prompted when QR is started the first time, where it will be saved to the QR settings afterwards avoiding it to popup again. The user though should still have the option to change the ContentCreatorName, because it might be a workstation that is used by several users.

I think we should have a hangout and talk about all that. Maybe this Thursday during the QIICR call?

[lassoan]

I think that ContentCreatorName should be something more meaningful, than just using getpass.getuser()

Yes, I think content creator name should be the real name of the operator.

it will be saved to the QR settings

That's what I'm trying to tell: don't put operator name in module-specific settings, because it will conflict with application-wide field, which will be added in the future. Instead, Create the application-wide field now and use it in QR.

When is the QIICR call?

[che85]

@lassoan When can we expect the application-wide field for the ContentCreatorName to be available in Slicer?

When is the QIICR call?

The QIICR call should take place coming Thursday, January 18th 2018 10am EST

[lassoan]

When can we expect the application-wide field for the ContentCreatorName to be available in Slicer?

As soon as you add it. For now, it would be just a value in Slicer.ini, in some general section. QR module would be the first one to use it.

We could also make it show up in the Application settings GUI, but you probably want to show it in QR module anyway, just to make sure it is correct.

[che85]

Ok sounds good.

One more question: What if there is more than one person using the same workstation with the same Slicer? Would it make sense to have a list of ContentCreatorNames? This way we could provide a dropdown in QR with all available names.

[pieper]

What if there is more than one person using the same workstation with the same Slicer?

One ContentCreator per OS login makes the most sense to me (application settings is per-OS-user). No reason to overcomplicate things by adding our own multi-user settings.

[che85]

Ok, just wanted to make sure.

[lassoan]

One ContentCreator per OS login makes the most sense to me

I agree. When someone implements HIPAA support in Slicer then there will be more sophisticated user identification.

Maybe we should add a method for get/set operator name in Slicer application class now. That would allow better control than just modifying application settings. For now it would just read/write application settings, but later it could be replaced by a more sophisticated mechanism.

[che85]

@lassoan did you get to work on the feature "support get/set operator name in Slicer application"?

[lassoan]

I haven't worked on this. Would you be able to implement it (just adding a method to get/set operator name in the Slicer application class) and send a pull request for review?

[che85]

Let me summarize:

We want to enable the user to edit the following general content information:

"ContentCreatorName" (will be auto-populated once https://github.com/Slicer/Slicer/pull/926is merged) "ClinicalTrialSeriesID" "ClinicalTrialTimePointID" "ClinicalTrialCoordinatingCenterName" "SeriesDescription" "SeriesNumber" "InstanceNumber"

For each segment, we track "SegmentAlgorithmType" and "SegmentAlgorithmName" (was integrated with https://github.com/QIICR/QuantitativeReporting/pull/226)

Now some more specific information regarding the workflow:

1. The user creates a new measurement report a. The system prompts general content information dialog with pre-populated information taken from the module settings.
2. The user can edit information and also can set defaults for every single meta information that will be saved in the module settings
3. User confirms dialog b. System caches entered information and uses those for generating the report later when requested by the User

Can we agree to this or are there any concerns?

[chribaue]

I think the dialog should pop up when the user clicks "save report" or "complete report" rather than at the beginning. Users who want to use the QR module without exporting to DICOM don't want to enter this information, or might not even know what it means.

[fedorov]

I agree with @chribaue

[che85]

Ok makes sense. Glad we clarified that!