[Octave-patch-tracker] [patch #9853] [octave forge] (image) implement niftiread, niftiwrite, niftiinfo

[Hartmut]

Follow-up Comment #16, patch #9853 (project octave):

I have had a close look at the files you linked in comment #14. Thanks for
preparing them! Here are some comments and wishes for improvements:

general:
* As mentioned before, I would prefer to have unit tests that also work
locally, without the need to download a sample file from the internet.
* There were some files in the github repo, that are unnecessary for an
inclusion into the image package. I have just ignored them: LICENSE, README,
.miss_hit . I just looked at the remaining 8 m-files.
* All error message strings should start with the name of the function that
issues this error message. Please also mention which if the input arguments
had a problem, if possible. For examle "myfunc: input I must be positive"
instead of "input must be positive".
* There are always examples in the help string. (This is nice.) Please add a
short text like "example code:" directly in front of these examples, so a user
can see that what follows is an example.
* Please also use "endif" instead of "end" at the end of a "if" section. This
is Octave coding style. (You can just search for "end" in the code, there
should not be any.)
* Please put a single space character between a function name and its
arguments, whenn calling a function, e.g. "disp (5)". But do not put this
space sign when indexing a matrix, e.g. "M(5)".
* 5 of the 8 m-files are helper functions. Those do not exist in Matlab. I
would suggest to make them private functions, e.g. by placing them in the
"private" subfolder of the "inst" code in the image repository. This way you
do not need to care about the user interface during future code changes. This
way it is also acceptable (in my opinion) to not have any unit tests in those
functions. An alternative would be to add several unit tests to all those
helper functions and then leave them around as "normal" user functions. Just
let me know how you would like to proceed here, I can the adapt this once we
push your new functions to the image repository.
* At some places, there are checks for OCTAVE_VERSION in the code (I found
this in nii2jnii.m and in niftiwrite.m). As part of the Octave image package
this is unnecessary. It would be better to remove these checks.
* The 3 main functions (niftiinfo, niftiread, niftiwrite) should have a lot
more unit tests.  Generally those tests should:
** cover all possible input syntax versions (and also some not-possible
versions to check for error messages)
** check all major use cases of the function, also all possible file types the
function is ment to deal with (e.g. zipped or non zipped)

comments on individual functions:
* niftiinfo:
** When I run the demo code in this new function under Matlab, then I get
slightly different results. I cannot judge which result is "correcter" in this
case:
*** spaceUnits = 0 (Matlab: 'unknown')
*** timeUnits = 0 (Matlab: 'None')
*** slicecode = 0 (Matlab: 'unknown')
*** transformname = Old (Matlab: 'None')
** The data type of header.ImageSize is "double" in Matlab and uint16 in this
code.
* nii2jnii: In the error message on line 101, it is not necessary to mention
"Octave to be installed" if the code becomes part of an Octave package.
* nitfiread: nothing additional to the general comments
* nitfiwrite:
** When I run the last demo code from the Matlab help page here:
https://de.mathworks.com/help/images/ref/niftiwrite.html and change the
info.Description property to something new, then niftiwrite, and afterwards
niftiread this. Then as a result I cannot find my individual Description
property in the read-in result. Is this correct?

I would be happy to eventually include this code into the image package. And I
hope the above comments help you to adapt your code for this purpose. (The
biggest work package is probably to add significantly more unit tests to the
three user visible functions. But this is important to help the code stay
usable in spite of a lot foreign future code changes in the image package as
well as in core Octave. So please invest this effort to make your code "ready
for future", no-one can do this better than you.)


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/patch/?9853>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/