-
Notifications
You must be signed in to change notification settings - Fork 494
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
ENH: Use display tables to allow showing derived and custom columns in DICOM browser #858
Conversation
cpinter
commented
Apr 9, 2019
•
edited by jcfr
Loading
edited by jcfr
- The displayed fields are in the same tables as the raw DICOM tags, with additional fields:
- Timestamp fields for insertion and displayed field generation
- Patients: displayed patient name (human readable), number of studies
- Studies: number of series
- Series: generic "size" column, and number of images and frames
- A new ColumnDisplayProperties table is added that contains the display properties for the patient, study, series tables:
- Displayed column name
- Visibility
- Weight: determines order, smaller values for columns towards the left ("heaviest sinks down")
- Format: format strings for dates and names for example (TBD)
This is a change originally implemented (partially) at the CTK Hackfest in 2013. It has now been resurrected and finished. It provides a more human readable (column headers, patient names, series descriptions, etc.), more usable (date added, number of studies, etc.), and more configurable DICOM browser. |
I plan make the changes in Slicer after this has been integrated. (Besides the CTK hash update it's just an additional Application Setting about the DICOM schema update, which I want to set to AskUser instead of the CTK default AlwaysUpdate). |
Really nice! FYI @nolden |
@@ -21,23 +22,44 @@ DROP INDEX IF EXISTS 'SeriesStudyIndex' ; | |||
DROP INDEX IF EXISTS 'StudiesPatientIndex' ; | |||
|
|||
CREATE TABLE 'SchemaInfo' ( 'Version' VARCHAR(1024) NOT NULL ); | |||
INSERT INTO 'SchemaInfo' VALUES('0.5.3'); | |||
INSERT INTO 'SchemaInfo' VALUES('0.6.1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 0.6.1
and not 0.6.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used 0.6.0 in a custom application, then realized that we had to make some more changes, the result is this version, 0.6.1.
Libs/DICOM/Core/ctkDICOMDatabase.h
Outdated
@@ -199,6 +199,9 @@ class CTK_DICOM_CORE_EXPORT ctkDICOMDatabase : public QObject | |||
bool createHierarchy = true, | |||
const QString& destinationDirectoryName = QString() ); | |||
|
|||
/// Update the fields in the database that are used for displaying information from information stored in the tag-cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to update the docstring to describe what is a "displayed field" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well as the relation ship with "weight". If this specific method is not the best place to document this, it could be added in the class documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if "displayed" is the best name.
May be "prettified" would be better or displayable
(already used in the ctkDICOMDisplayedFieldGenerator
classname) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that "prettify" is even a word, but apparently it is: "make (someone or something) appear superficially pretty or attractive."
I think "displayed" or "displayable" captures the intent better, which is converting from machine-readable data to text that can be displayed to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think "displayed" or "displayable" captures the intent better
👍
Libs/DICOM/Core/ctkDICOMDatabase.h
Outdated
void schemaUpdated(); | ||
|
||
/// Trigger showing progress dialog for display fields update | ||
void displayFieldsUpdateStarted(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
displayedFieldsUpdateStarted
or prettifiedFieldsUpdateStarted
|
||
/// \ingroup DICOM_Core | ||
/// | ||
/// \brief Generates displayable data fields from DICOM tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be neat to describe here the role of the class was well as the relationship with the "rules" (associated with ctkDICOMDisplayedFieldGeneratorAbstractRule
)
|
||
/// \ingroup DICOM_Core | ||
/// | ||
/// Abstract base class for generating displayed fields from DICOM fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indicate here that this class works with ctkDICOMDisplayedFieldGenerator
|
||
#define EMPTY_SERIES_DESCRIPTION_RTPLAN "Unnamed RT Plan" | ||
#define EMPTY_SERIES_DESCRIPTION_RTSTRUCT "Unnamed RT Structure Set" | ||
#define EMPTY_SERIES_DESCRIPTION_RTIMAGE "Unnamed RT Image" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of macro, what about defining a const field ?
@@ -223,6 +223,7 @@ class CTK_DICOM_CORE_EXPORT ctkDICOMItem | |||
/// \brief Nicely formatted (group,element) version of a tag | |||
/// | |||
static QString TagKey( const DcmTag& tag ); | |||
static QString TagKeyStripped( const DcmTag& tag ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method required ? Should TagKey be systematically stripped ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TagKey returns the tag in parentheses. TagKeyStripped does not have the parentheses. I don't think the original one could be removed. However, maybe the name of the new function is not good enough. If you have a better suggestion let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about TagKeyWithoutParenthesis
? Or may a static function called StipParenthesis
could be added ? And the same effect would be achieved doing StipParenthesis(TagKey(tag))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about TagKeyUnpunctuated
or TagKeyWitoutPunctuation
and unpunctuateTagKey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TagKeyWithoutPunctuation
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like TagKeyUnpunctuated and TagKeyWithoutPunctuation. There is still a comma between the numbers. Is it punctuation? Because if it is then this name too would be somewhat misleading.
The parentheses version sounds a bit strange to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also be fine keeping TagKeyStripped
, let's just make sure to add a docstring explaining what the function is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, if it only changes removes the parentheses and not the comma then I'd change my vote to TagKeyWithoutParenthesis
but am also fine with TagKeyStripped
with documentation.
Just for reference, in similar javascript code we use punctuateTag
and unpunctuateTag
. to go between (XXXX,XXXX), used in the dcmtk-derived data dictionary and XXXXXXXX used in DICOM JSON Model. My current thinking is that these are close to the standard which uses "tag" made of group and element, but there's no reason to change the usage in CTK at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR keeping TagStripped but with explanation. I can still change it to TagKeyWithoutParenthesis if it's better for everyone.
Thanks for the review @jcfr ! I'll address the cmoments tomorrow. So is there anything to do with the ...DisplayFields... functions/variables/comments? |
Few comments on the display fields:
|
e3e5bea
to
479a6da
Compare
We thought about this, and decided that the average user probably won't understand what are instances, and they are almost always images
Both are fine with me and we can change it, but then I'd need to increase the schema version again. |
479a6da
to
86b7e9d
Compare
The original idea of the DICOM database to strictly store only original DICOM values. This resulted in a barely usable DICOM browser with severe limitations. Due to inconsistencies and complexities of DICOM and how it is implemented in various software, the raw DICOM field values are not meaningful for users. They need to be processed, merged, interpreted for the user before displaying them. For example, for some RT IODs, "Series description" field is always empty and a completely different field is used for storing series description. "Description" field fill rules can construct the description from fields depending on what kind of series is that, what device generated it, what other fields are specified, etc. Similarly, we would not like to show number of instances to the user. Instead, we want to report the number of slices (regardless of a volume is stored in an enhanced format in a single file or in a legacy format in one file per slice). So, the difference in names is not just for sake of simplification but also to indicate that it is not a 1-to-1 mapping to some DICOM field. |
The current implementation actually does show the number of instances that belongs to the series, but this is one of the few things we'd like to improve later. About the series description: that's right, if we keep the name it might suggest that it contains the SeriesDescription tag, which is not always true (which is currently in case of some RT-specific modalities). |
About "number of images" vs "number of instances" - I think it should be quite easy to conceptualize "instance" for the user. The advantage is that the column name would be more generic and consistent with the content of that column for a variety of series types. Considering your explanation, I agree it makes sense to deviate from direct mapping between column names and DICOM attribute names. So "instance" may not map to the "DICOM instance", but refer to the instance of "thing" being counted. For example, in RTSTRUCT - you probably would want to count contours, or structures - does it make sense to refer to either one as "image"? In SEG - you would count frames, or segments, probably not images. What would "Number of images" mean when referring to RWWM or SR modalities? I just think it is an unnecessary and confusing simplification. EDIT: Another "neutral" name for that column could be "Count" or something of that kind. About "series description" and "study description". Adding "Study" and "Series" for the "Description" would not imply they map one-to-one to the DICOM content, but would just make the appearance of the table more consistent. For example, you already say "Study ID", "Series #", "Number of Studies", "Number of Series" - it just looks inconsistent to drop that specification for the "Description" column. |
As I think about this, this approach of not mapping attributes one-to-one to DICOM is great if someone is using Slicer and nothing else. But I think it will be quite confusing for the users who work with different tools, as the table view they will see is likely to be very different in Slicer from other tools that (I think) just display DICOM attribute values. Did you think about this? Did you consider supporting a configuration mode for the DICOM browser that would allow to switch to the behavior consistent with non-Slicer tools? |
The current customizations of fields are only for cases where
So no field is really "changed" in the DICOM browser. I don't think it will cause a problem when using different tools with this initial version. About the column names:
I'll wait until @pieper @lassoan and maybe @jcfr chime in, and then make any necessary changes if the consensus is to change these. |
Right. I am just thinking about the situation where user has certain expectations about what to see in a certain field, based on viewing it in another tool DICOM table, and then they see something different (which could be much much more informative!) in Slicer ... Maybe a related suggestion is to somehow mark those fields that have been altered?
By "instance" I meant a generic countable item stored in a DICOM series, which would be modality-specific. Maybe "Count" or even "#" is a better and more neutral name instead of "Number of images" or "Number of instances"? Maybe it would it be possible to show in tooltip what this number refers to for the specific series? |
I don't know any reasonably usable software that would not separate the patient browser from DICOM metadata view. Do you know any? Then we could learn from that. If anybody is interested in raw DICOM fields then the metadata browser is there.
I agree that "images" term is simplification (what if it is an RTPLAN, etc.). We thought it should be acceptable, since this term has been used for example in OsiriX for many years (https://www.osirix-viewer.com/wp2015/wp-content/uploads/2015/09/Database-1024x576.png). We could use another word, but I would avoid "instance", since that would mislead users to think that it is raw DICOM value. What columns names are displayed in software that you have access to? |
I think there is a misunderstanding on either my or your side. An example of scenario I had in mind is the following:
I think the above could cause confusion.
Avoiding instances is fine with me. "Count" or "#" could be a neutral name. |
What about adding an small icon on the right side of the "Description" in the column header. Mouse over the icon and it displays how the text was generated. |
How about this:
|
Thinking a bit more, I think '#' won't be good after all. We use that symbol for objects' number identifier. For example we have 'Series #' in the same table and also 'Echo #'. What about 'Count' in the column header and 'DisplayedCount' in the schema. Note that a version bump will be needed after all. |
…n DICOM browser - The displayed fields are in the same tables as the raw DICOM tags, with additional fields: - Timestamp fields for insertion and displayed field generation - Patients: displayed patient name (human readable), number of studies - Studies: number of series - Series: generic "size" column, and number of images and frames - A new ColumnDisplayProperties table is added that contains the display properties for the patient, study, series tables: - Displayed column name - Visibility - Weight: determines order, smaller values for columns towards the left ("heaviest sinks down") - Format: format strings for dates and names for example (TBD) ----------------- Original commits: ENH: Add option to confirm Remove action ENH: Add autoSelectSeries and selectionMode properties to DICOM table manager - autoSelectSeries property controls whether or not series are automatically selected when selection changes in the studies table - selectionMode can be single or extended selection. If autoSelectSeries property is on and single selection is active, then the first series is selected from the list ENH: Emit schemaUpdated when all steps are done; Reduce updates - schemaUpdated signal is now emitted as before: once all the schema update steps are finished - Column swaps now do not trigger geometry update on each swap in applyColumnProperties, which can be dozens of times - Make ctkDICOMTableView::tableView callable from python ENH: Add progress dialog for display fields update - Display fields update has now a progress dialog, implemented similarly to schema update and import - Coding style fixes ENH: New fields in DICOM database, display fields update speedup, schema update option - Add new size and number of frames fields - Considerable speedup of updateDisplayedFields by wrapping the apply step (update queries) in a transaction - Add property in DICOM browser for schema update option, which determines whether to always update schema, never do it, or ask user. Default is always update - Fix typos and style ENH: Add patient and study numbers to DICOM tables ENH: Allow setting column display properties Column display properties of the DICOM database can now be set externally. When it happens, then - Update ColumnDisplayProperties table in database - Trigger applyColumnProperties to update table view (header name, visibility, and order for columns) Patient sex is now a visible column in the patient table by default ENH: Add support for DICOM table column weights The column with the lower weight is towards the left. Defaults are set, which show the same order as before, except for two changes - Human-readable patient name is on the left in the patients table and the raw patient name is not visible any more - Date added column is the rightmost in all tables ENH: Generate human-readable patient name ENH: Add calculation of number of images in series STYLE: Document displayed field generator classes ENH: Apply column display properties in DICOM table view - DICOM table view applies column display properties from database - Clean up style in ctkDICOMTableView ENH: Column properties initialization - Fill ColumnDisplayProperties table with the defaults - Weight and Format fields are not yet utilized! - Add functions to get column display properties - Rearrange functions in ctkDICOMDatabase so that Private functions are together - Further cleaning up style ENH: Set displayed fields updated timestamps - Type of DisplayedFieldsUpdatedTimestamp field changed to DATETIME for easier comparison when doing SELECT - Set the value of the field when applying the displayed fields - Fix many of the indentation problems (there are 3 different styles in this one class alone) ENH: Add DICOM displayed fields in the original database tables - Instead of separate tables, the displayed fields are now in the same tables as the raw DICOM tags, with additional timestamp fields for insertion and displayed field generation - A new ColumnDisplayProperties table is added that contains the display properties for the patient, study, series tables: - Displayed column name - Visibility - Weight: determines order, smaller values for columns towards the left ("heaviest sinks down") - Format: format strings for dates and names for example (TBD) ENH: DICOM browser shows display tables ENH: Pass cached tags by reference to DICOM displayed field roles ENH: Use display tables to allow showing derived and custom columns in DICOM browser !WIP! Original commits: COMP: Fixed merge issues in display table merge, builds fine Fixed bug when split DICOM data was inserted one after the other in Display* tables Deletion from database works for display tables too; Some bugfixes Inserting one patient/study/series has the expected result in the Display* tables Fixed function signatures for getting display fields Changed way of getting the display field maps Generate and use internal patient UID for the display tables WIP Add UIDs in the display tables ctkDICOMDatabase::getCachedTags changed to return empty string if the cached field is empty or the field is not in the instance Fixed rules (still WIP) Added merge helper functions Added initial implemntation for default displayed field generator rule Empty field names are passed to the mergeDisplayFieldsForInstance function Added empty names maps; Implemented getDisplayFieldsForInstance for the RT series description rule Changed DisplayPatients table schema: renamed MRN to PatientID; Added role headers in the CMakeLists file Completed rule interface Fixed new files query for updateDisplayedFields; Added applyDisplayFieldsChanges method to insert or update display fields after generation is done Added skeleton of ctkDICOMDisplayedFieldGenerator and a few example rules Added skeleton for ctkDICOMDatabase::updateDisplayedFields Input data assembled and passed to the displayed field generator (not tested) Update display fields method body implemented Added display tables to DICOM database schema; Merged from lassoan/276-dicom-database-display-tables
86b7e9d
to
9c2af28
Compare
I pushed the changes that prefix Description with Study/Series and rename 'Number of images' to 'Count'. If this is fine for everyone, I'd appreciate if this could be merged soon. Thanks a lot! |
Works for me. I suggest we integrate at 4.30pm EST today. |
No concerns, thanks! |
Excellent, thanks! I'm doing a test build with Slicer then will update it to contain the CTK changes along with the new application setting for the DICOM database. Please let me know if there are any issues with this one, or ideas for later. |
Once you have examples / design patterns how DICOM plugins should initialize the new modality-specific column items, would be great if you could update this issue with the pointers! |
- DICOM browser is now customizable and shows more informative fields. More information at commontk/CTK#858 - Add application setting for DICOM database update option. By default the user is asked whether they want to update or create new git-svn-id: http://svn.slicer.org/Slicer4/trunk@28142 3bd1e089-480b-0410-8dfb-8563597acbee
The CTK hash has been updated (and the new app setting added) in Slicer/Slicer@75fceb2 I'll add information about how to create new plugins and what they can do. Jc suggested this page, and I think it's a good place for this: https://www.slicer.org/wiki/Documentation/Nightly/Modules/DICOM#Information_for_Developers |
FYI I updated the wiki page mentioned above. |
Thanks Csaba. If you would like to more easily reference classes in wiki text, you could use the doxygen template. See https://www.slicer.org/wiki/Documentation/Nightly/WikiCheatSheet#Doxygen Examples are available here: https://www.slicer.org/wiki/Template:Documentation/Nightly/doxygen-class-url |
- DICOM browser is now customizable and shows more informative fields. More information at commontk/CTK#858 - Add application setting for DICOM database update option. By default the user is asked whether they want to update or create new git-svn-id: http://svn.slicer.org/Slicer4/trunk@28142 3bd1e089-480b-0410-8dfb-8563597acbee