Reviews

Rupert Brooks

Thursday 20 April 2006

Summary:

This paper is a revised version of one submitted previously. It is the submission of classes to read and write MINC2 format files within the ITK pluggable factory framework.

This submission is significantly improved from the last version. I will incorporate this code in my code tree and begin using it regularly. Despite a few issues, I am confident enough to trust it on simple cases. However, I recommend a good code polishing and more extensive test suite.

Hypothesis:

n/a

Evidence:

A test which reads a minc2 file (provided), prints out a coordinate from it, and writes it out again is provided. There is no numerical testing of the result, it is presumed that the reader will view the output themselves. In fact, the provided file is rather anomalous, as its direction cosine vectors are not normalized - if you normalize them, then it seems that the directions are just along the regular axes anyway, so i am not entirely sure that this tests what it is supposed to. However, I tested it on a file where the direction cosines are normalized and not aligned with the coordinate axes and it works fine.

Open Science:

Source code is provided. This work will, in effect, make two large bodies of open-source medical imaging software interoperable and so is very useful to open science.

Reproducibility:

I downloaded and compiled the code. I did not actually test the CMakeLists, i just plugged the code into my own code tree where i had a place for it.

One edit was required to get it to compile under Visual C++7. There were some moderately serious problems with the MINC writing part. The code had some nasty access violations - I fixed the issues enough to get the test working, but i suspect the code may still need a little polishing. See code quality section below.

Use of Open Source Software:

The work makes ITK read the open-source minc data format. It is entirely open source.

Open Source Contributions:

The work is open source.

Code Quality:

There were some minor difficulties compiling this code on Windows. I also had some memory access violations on the write. I tweaked it and got it to work with the following changes. From the tweaking, my overall impression is that the code is good, but not yet perfect.

In particular, I would expect problems when facing files where the data is not scalar or the number of dimensions is not 3.

I suspect there are memory leaks in the code as well, some \\\"new\\\"s do not have corresponding deletes. See the first changes in the .cxx file below.

Here are my diffs to the code, interspersed with comments about why.
bash-3.00$ diff /cygdrive/c/temp/itkMINC2ImageIO.h ../src/IO/itkMINC2ImageIO.h
132c132,133
< int m_DimensionIndices[5];
---
> // int m_DimensionIndices[5];
> int * m_DimensionIndices;

I changed this while i was fixing the access violations. The declaration of DimensionIndices was inconsistent with the declaration of the other Dimension parameters.
bash-3.00$ diff /cygdrive/c/temp/itkMINC2ImageIO.cxx ../src/IO/itkMINC2ImageIO.cxx
239a240
> m_DimensionIndices = new int[MINC2_MAXDIM+1];

This is to make the allocation of m_DimensionIndices compatible with the other dimension parameters. I didnt put in a corresponding delete, because i was not sure where to put it. Im not sure where the other dimension parameters are deallocated. so there is probably a memory leak.
241c242,243
< for (int i = 0; i <= MINC2_MAXUSE; i++)
---
> // for (int i = 0; i <= MINC2_MAXUSE; i++)
> for (int i = 0; i <= MINC2_MAXDIM; i++)

Im not sure what the difference between MAXDIM and MAXUSE is. Further in the code, there is a loop over MAXDIM and the fact that m_DimensionName was not fully initialized caused problems.
610c612
< buffer += Strides[1];
---
> //buffer += Strides[1]; 612c614,615
< buffer += Strides[2];
---
> //buffer += Strides[2];
> //std::cout<<\\\"max \\\"<

Originally there was an access violation in here. I had all kinds of problems with this function. Looking at Strides in the debugger, it doesnt look at all like i think it should and besides, if you increment buffer... why increment it by Strides again later. Anyway, i just made it increment by one each time through the loop, and all seems to work. But ultimately, i see why the author wants the more sophisticated approach - maybe this could be handled on the ITK end with an iterator?
1179c1182
< unsigned long coords[m_NDims];
---
> unsigned long * coords=new(unsigned long[m_NDims]);

This makes it compile on Windows. No, i did not put in the corresponding delete.

Applicability to other problems:

MINC is widely used, so there are likely many projects that can make use of this work.

Suggestions for future work:

Ultimately, it would be nice to have this completely integrated in the pluggable factory design for ITK. It simply involves including itkMINC2ImageIOFactory.h and adding the line

ObjectFactoryBase::RegisterFactory( MINC2ImageIOFactory::New() ); to the itkImageIOFactory.cxx file and should be done when this code is integrated.

Requests for additional information from authors:

n/a

Additional Comments:

When testing this code, I was confused at first because itk::Image does not support direction cosines at all, and one must use itk::OrientedImage to do so. This is done correctly in this project, and mentioned in comments in the test file, but it might be worth drawing the readers attention to it more specifically.

The paper does not follow the itkJournal typographical conventions.

The test file provided has direction cosines that do not have magnitude 1

Minor spelling and grammar issues:

• wed site should be web site
• miset_real_value_hyperslsab should be miset_real_value_hyperslab
• are copied into a temporary buffer one by one and send to - should be sent to.