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

MINC support should be enabled in all ITK upstream tools #75

Open
gdevenyi opened this issue Sep 5, 2016 · 10 comments
Open

MINC support should be enabled in all ITK upstream tools #75

gdevenyi opened this issue Sep 5, 2016 · 10 comments

Comments

@gdevenyi
Copy link
Contributor

gdevenyi commented Sep 5, 2016

Just a note that I'm increasingly getting people asking about using our various MINC atlases with tools such as ITK-SNAP and 3DSlicer https://github.com/Slicer

They seem to not be interested in touching minc-tools convert files.

Since these tools are built with ITK underneath, it shouldn't be "too hard" to enable MINC support at a build/interface level.

@andrewjanke
Copy link
Member

This is an ongoing problem. One technique I have used successfully in the past is to offer a "basic" version of the atlas in Nifti format but an "enhanced" version in MINC format that includes things like label identifiers, full names of structures, muti-modality data etc. Be sure to also point out that for large models (multi-GB) MINC is the only real option.

In time they do recognise the benefits of using HDF5 over other formats but it takes time.

@thewtex
Copy link
Member

thewtex commented Sep 6, 2016

@vfonov has does a great job developing the ITK-MINC ImageIO bridge, and it is mature at this point. It could be enabled by default by removing this line:
https://github.com/InsightSoftwareConsortium/ITK/blob/c83b067dd781a5d7d114ca54cacbdd507da7ec64/Modules/IO/MINC/itk-module.cmake#L16

However, someone needs to take on the responsibility to address any issues that come up on the ITK dashboard:

https://open.cdash.org/index.php?project=Insight

@gdevenyi
Copy link
Contributor Author

A followup here, Slicer3D is trying to integrate MINC into its release, they're having some build issues, discussed here http://www.na-mic.org/Bug/view.php?id=4085 (I was informed here CoBrALab/atlases#6 (comment) )

It looks like it's a windows build issue which is probably something we haven't touched much before, @rdvincent or @vfonov have you done much testing with windows?

@gdevenyi
Copy link
Contributor Author

Following up, these are also issues with getting MINC enabled as default on ITK tools:
https://open.cdash.org/viewTest.php?onlyfailed&buildid=4545427
https://open.cdash.org/viewDynamicAnalysis.php?buildid=4545685

I suspect that @rdvincent has already fixed some of these in devel

@vfonov
Copy link
Member

vfonov commented Sep 12, 2016

Looks like they built HDF without zlib support.

@vfonov
Copy link
Member

vfonov commented Sep 12, 2016

Yes, this error have been addressed: https://github.com/BIC-MNI/libminc/blob/develop/libsrc/minc_config.c#L99

@thewtex
Copy link
Member

thewtex commented Sep 12, 2016

Looks like they built HDF without zlib support.

ITK's internal HDF was recently updated to the latest upstream release, but zlib support was mistakenly dropped in the update. It is added again here:

http://review.source.kitware.com/#/c/21511/

@blowekamp
Copy link
Contributor

Last time I ran valgrind on MICIO there where quite a number of memory defects detected. Once the memory defect and coverage improves some on the MINCIO modules I would not object to enabling it on ITK be defaults.

Once this HDF issue get cleared up I'll can run valgrind again.

@rdvincent
Copy link
Member

Many, though perhaps not all, memory defects have been fixed recently. I have been running with address sanitizer/leak sanitizer enabled when I test. I'll be happy to receive any feedback you have.

@blowekamp
Copy link
Contributor

These may just be some minor problems with the ITK class now.

I ran valgrind this morning:
https://open.cdash.org/viewDynamicAnalysis.php?buildid=4545685

On the tests that pass there are some leaks here:
https://open.cdash.org/viewDynamicAnalysisFile.php?id=4096830

It looks like the test that are failing are failing with a thrown exception. Ideally even exceptions cases should not leak memory either:
https://open.cdash.org/viewDynamicAnalysisFile.php?id=4096825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants