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.

Sign up for GitHub

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

Jump to bottom

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

Open
rfloca opened this issue Dec 20, 2019 · 13 comments · May be fixed by #493
Open

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

rfloca opened this issue Dec 20, 2019 · 13 comments · May be fixed by #493
Labels
enhancement

Comments

@rfloca
Copy link

rfloca commented Dec 20, 2019

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.
@rfloca rfloca changed the title Itkimage2dcmSegmentation and itkimage2paramap should support "in memory" DCM sources. Refactor itkimage2dcmSegmentation and itkimage2paramap to support "in memory" DCM sources. Dec 20, 2019
@fedorov
Copy link
Member

fedorov commented Dec 21, 2019

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
Copy link
Author

rfloca commented Dec 23, 2019

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
Copy link
Member

fedorov commented Jan 13, 2020

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

@fedorov
Copy link
Member

fedorov commented Jan 23, 2020

@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
Copy link
Author

rfloca commented Feb 12, 2020

@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!

@rfloca rfloca mentioned this issue Jan 10, 2024
Closed
@michaelonken
Copy link
Member

michaelonken commented Jan 11, 2024 •
edited
Loading

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 NA-MIC/ProjectWeek#881 before putting hands on the function signatures.

@rfloca
Copy link
Author

rfloca commented Jan 11, 2024

@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 fedorov mentioned this issue Jan 17, 2024
Closed
@fedorov
Copy link
Member

fedorov commented Jan 29, 2024

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.

@fedorov fedorov mentioned this issue Jan 30, 2024
Closed
@fedorov fedorov linked a pull request Feb 15, 2024 that will close this issue
WIP: Use DcmItem in API where possible. #493
Draft
@rfloca
Copy link
Author

rfloca commented Oct 21, 2024

Hi everybody. I hope you are fine.

This problem came up again in some projects and the MITK mailing list. I would like to know what the status is here. If I remember correctly @michaelonken said in the VCon in January, that he could refactor it swiftly. But maybe it was already done and I missed it 🙈

@michaelonken
Copy link
Member

Hi Ralf,

I've done this already in some branch (don't have it at hand right now) but our main problem is that we cannot use a newer DCMTK in dcmqi since it will then class with the DCMTK libraries brought in by ITK dependencies. This is currently under investigation (for some time now...). I don't know the current status, maybe @fedorov knows more.

@fedorov
Copy link
Member

fedorov commented Oct 21, 2024

@jcfr @jadh4v do you have any updates on the status of fixing CMake issues and updating DCMTK in dcmqi and Slicer?

@rfloca
Copy link
Author

rfloca commented Oct 22, 2024 •
edited
Loading

Hi Ralf,

I've done this already in some branch (don't have it at hand right now) but our main problem is that we cannot use a newer DCMTK in dcmqi since it will then class with the DCMTK libraries brought in by ITK dependencies. This is currently under investigation (for some time now...). I don't know the current status, maybe @fedorov knows more.

@michaelonken Thanks for the information. From which version on DCMTK has problems with ITK? So far we witnessed no problems with an external DCMTK used in ITK and MITK to build upon.

@michaelonken
Copy link
Member

I think we tried to link a new DCMTK (such as one of the current commits on master) by specifying it in the dcmqi build process.

When linking the linker complains that there are two versions of DCMTK libs around, e.g. dcmdata, one from the dcmqi link and one from (as fas I as could see) ITK, which brings its dependencies into the dcmqi link process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP: Use DcmItem in API where possible. michaelonken/dcmqi
3 participants
@fedorov @michaelonken @rfloca