search

QIICR / dcmqi

dcmqi (DICOM for Quantitative Imaging) is a free, open source C++ library for conversion between imaging research formats and the standard DICOM representation for image analysis results
https://qiicr.gitbook.io/dcmqi-guide/
BSD 3-Clause "New" or "Revised" License
233 stars 62 forks source link

Refactor itkimage2dcmSegmentation and itkimage2paramap to support "in memory" DCM sources. #390

Open rfloca opened 4 years ago

rfloca commented 4 years ago

Status: Currently the interfaces of Itkimage2dcmSegmentation and itkimage2paramap build up a dependency to DcmDataset as you pass a vector of DcmDataset* representing the dicom source images.

Proposal: As far as I can see in the implementation of dcmqi only parts of the class interface is used that is directly inherited from DcmItem. If this is correct and I am not mistaken this would mean that you could easily switch to the following signatures:

    static DcmDataset* itkimage2dcmSegmentation(vector<DcmItem*> dcmSourceItems,
                                                vector<ShortImageType::Pointer> segmentations,
                                                const string &metaData,
                                                bool skipEmptySlices=true);

    static DcmDataset* itkimage2paramap(const FloatImageType::Pointer &parametricMapImage, vector<DcmItem*> dcmSourceItems,
                                        const string &metaData);

It would not break existing code, but ensures the possibility to use the methods, when you have no dcm files but all informations you need. As it is e.g. in some of our use cases, where we have the values of the dcm tags at hand but not the original files. The change in the function signature would gurantee that one can directly populate DcmItems in memory and pass them instead of trying to get the files or "simulate" them by hacking temporary files with the needed information; because nothing DcmDataset-specific is used.

Have I missed something? What do you think.

If the change proposal is supported and there is reasoning against it, we could also provide a patch sooner then later, because it would ensure that we do not implement against implementation details that might change.

Looking forward to read your thoughts.

fedorov commented 4 years ago

Ralf, thank you for reaching out! I know you guys brought this use case several times, and I do want to support it. Over a year ago now, we started working on complete refactoring of dcmqi, to address this issue, among many others - see #226. Very unfortunately, we ran out of steam (and also got bogged down into the issues related to lacking experience with Git workflows while developing a branch collaboratively with Christian Herz...) although I thought we were quite close.

Let me think about this, I have not touched or looked at that code in a while!

rfloca commented 4 years ago

Hi Andrey,

thanks for your swift response. Also thanks for üointing out the other issue, looks realy interesting. Let me know what you think about the proposed mitigation strategy (I think, if finished, the approach in #226 is far better and more sophisticated), as soon as you have time to think about it. But before that, have a merry christmas.

fedorov commented 4 years ago

@rfloca I did not forget, sorry for not responding. I will try to get to look at it this week.

fedorov commented 4 years ago

@rfloca sorry again for the unacceptable delay...

I looked into your proposal, and I don't see any problem with what you are suggesting. I also looked into the refactoring effort that I mentioned earlier, and I can't tell you if or when I will get to it - it will be a significant effort to revive that PR. What you are suggesting is sensible, and I think should not be disruptive, so I would definitely welcome your patch with the functionality you proposed!

rfloca commented 4 years ago

@fedorov no worries. I was bussy myself (as you can see due to my delay)

Thanks for the feedback. I will make a pullrequest with a patch as soon as possible!

michaelonken commented 8 months ago

Just my 2 cents after quick-checking the current code:

It should be possible switching to DcmItem instead which is generally more "flexible" than using a DcmDataset. We could be able to refactor this quickly without problems.

However, I suggest to wait for the discussion regarding https://github.com/NA-MIC/ProjectWeek/issues/881 before putting hands on the function signatures.

rfloca commented 8 months ago

@michaelonken Thanks for looking into that matter and confirming the assumption. I agree that we should wait for the results of the discussion and then see how to move forward. Looking forward to the exchange at the project week.

fedorov commented 7 months ago

I have no objection to changing the signature. I am not aware of any other tool that uses dcmqi API and not command line converters other than MITK, so I doubt it will cause much trouble.