patchGNU Octave - Patches: patch #9853, [octave forge] (image) implement...

 
 

patch #9853: [octave forge] (image) implement niftiread, niftiwrite, niftiinfo

Submitter:  Qianqian Fang <fangq>
Submitted:  Fri 13 Sep 2019 04:52:26 AM UTC
   
 
Category:  Forge : new function Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 24 Sep 2022 04:11:36 PM UTC, comment #17: 

@Hartmut, thanks a lot for your detailed feedback. I will walk down the list and fix each of the mentioned issues.

for the question regarding whether to place helper functions inside `private` folder, my preference is to add unit tests and define those as normal functions. I think they are more intuitive to use than letting users write their own.

will post my updated version once I am done.

Qianqian Fang <fangq>
Sat 24 Sep 2022 04:02:21 PM UTC, comment #16: 

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.)

Hartmut <hardy>
Wed 10 Aug 2022 06:26:47 PM UTC, comment #15: 

Thanks for your efforts to adopt your code to Octave.

To your question: I would prefer to have a BIST test included, that works without internet connection as well. And since the test is ment to make sure that the functionality of your code survives (most) future code changes, I think a locally generated file should do the job as well.

I am also willing to review this code and give you detailed feedback on it. But this will unfortunatelly take several weeks because I will be offline during the next weeks. If anyone else manages it before me to do some review, I am happy with this, too.

Hartmut <hardy>
Tue 09 Aug 2022 04:15:14 PM UTC, comment #14: 

I just committed an initial set of units with octave-styled docstrings at

https://github.com/NeuroJSON/jnifty/tree/octave9853

Please let me know if you see anything that you want me to tweak - one question I have is that some of the units requires nifti data files to do demo/tests, I am currently using

urlwrite('https://nifti.nimh.nih.gov/nifti-1/data/minimal.nii.gz', [tempdir 'minimal.nii.gz']);

to download a standard sample to tempdir, but this requires Internet connection, and it seems to be slow to download.

if we don't have to use official sample, I can also locally generate sample files, such as

https://github.com/NeuroJSON/jnifty/blob/octave9853/nii2jnii.m#L60-L63

let me know what you think. I ran some quick tests on octave 4.2.2 (Ubuntu 18.04) and seems to be working ok

Qianqian Fang <fangq>
Sat 06 Aug 2022 07:29:02 AM UTC, comment #13: 

I am happy to hear that there is movement in this topic, again.

In the image package, we generally try to adhere to the Octave style guide that you mentioned. This includes the # for comments as well as endfor etc. instead of a simple but umbigious end. (But I do not know of a template file, sorry.)

Hartmut <hardy>
Fri 05 Aug 2022 09:25:06 PM UTC, comment #12: 

just to let you that I have restarted working on code style reformatting.

the latest version can be found at

https://github.com/NeuroJSON/jnifty/tree/octave9853

the files included are the minimal set as discussed in comment #5, where I added fopen('z') and removed zmat/jsonlab dependencies (although, if they are installed, they can also be called).

I am using miss_hit (https://github.com/florianschanda/miss_hit) in the octave mode (--octave) to do as much automated formatting as possible. To tune the miss_hit config rules, I would like to get some pointers on the formatting style

So far, the current miss_hit formatting rules have done the following:

1. set indentation to 2 spaces
2. adding white spaces after comma and around operators

I might have to do some of the remaining formatting manually. Before that, I want to know the following:

Which of the styles in https://wiki.octave.org/Octave_style_guide are absolutely required? is endfunction/endif/endfor required? is using '#' instead of '%' required? if someone can point me to a template file, that would be very helpful. thanks

Qianqian Fang <fangq>
Fri 05 Aug 2022 07:12:23 PM UTC, comment #11: 

Packages can have optional ("suggested") dependencies. E.g., he mapping and io package have those [*]. And of course core Octave has plenty of them.

That the image package is "self-supporting" til now is of course Good (and laudible, I like that); but from a practical POV I wouldn't object to introducing suggested dependencies in any package with such a wide functionality as the image package.
If specific dependencies for a few specific functions weren't available when the package got built, most of the package still works and/or most users can still use the package successfully.
Of course, the contributor of those specific functions should make sure that missing deps. are gracefully catched during package installation and -use.

[*] maybe not formally announced, but the mapping package makes use of GDAL and the io package has a large collection of optional Java dependencies.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 05 Aug 2022 03:47:35 PM UTC, comment #10: 

comment #9:

> I'm a bit worried that adding this would add too many dependencies to the image package when at the moment it has none. Also, since I feel this is a quite specialised topic, a separate nifti package may be more suitable.


hi Carnë, sorry for the delay - does the dependencies listed in comment #5 acceptable? while I am enthusiastic on the JSON/JNIfTI format, I am happy to just make the plain nifti support available to octave - and make it compatible with matlab.

Qianqian Fang <fangq>
Fri 05 Aug 2022 09:35:09 AM UTC, comment #9: 

I'm a bit worried that adding this would add too many dependencies to the image package when at the moment it has none. Also, since I feel this is a quite specialised topic, a separate nifti package may be more suitable.

Carnë Draug <carandraug>
Group Member
Thu 30 Jan 2020 06:46:26 AM UTC, comment #8: 

hi Carnë, you can find .nii or .nii.gz files in many websites,

for example

https://github.com/neurolabusc/MRIcroGL/tree/master/Distro
https://nifti.nimh.nih.gov/pub/dist/data/nifti2/

I also included a few my tools for testing purposes

https://github.com/fangq/jnifti/tree/master/samples/headct
https://github.com/fangq/brain2mesh/tree/be391ecf1d225e9b5b4a4b0cab2078a055774ddd/examples/SPM


I am happy to convert my additional toolboxes into octave-forge packages. I just need someone to point me to a document regarding the formats for proposing a new octave-forge package.

to optionally support JNIfTI (JSON/JData based nifti files), my functions requires JSONLab - not just jsonencode and jsondecode, but jdataencode/jdatadecode as well as binary JSON (UBJSON) support.

currently, jsonencode/decode was discussed in https://savannah.gnu.org/bugs/?53100, which does not handle encode/decode JData formats (http://github.com/fangq/jdata).

in the following week, I may have some free time during the evenings of a conference, so I will start working on formatting the proposed units into Octave required comment formats. I will post those when I am done.

Qianqian Fang <fangq>
Wed 29 Jan 2020 06:02:39 PM UTC, comment #7: 

Could you link some nift sample files and expected output so that we can try and expand on i.

About the dependency on other packages, if the dependency is of Octave code, then the Octave code needs to be on  an Octave Forge package which you may not desire.

For json, I guess octave core should implement jsonencode and jsondecode and then you could use those?

Carnë Draug <carandraug>
Group Member
Tue 22 Oct 2019 06:32:21 PM UTC, comment #6: 

I do like the proposal from comment #5. (Assuming that it is still a significant benefit to have a niftiread function without any json support, which I cannot judge at all.)

@Guillaume: If you are willing to take on the burden of adapting Qianqian's code as outlined in comment #5, I think this could probably be a good extension of Octave's image package. Please ask here when any implementation questions come up while doing this.




Hartmut <hardy>
Mon 21 Oct 2019 10:38:30 PM UTC, comment #5: 

@Guillaume

if reading/writing .gz can be resolved using the fopen('z') flag and support to JSON format is not a high priority, then, I think the only needed units are

  • niftiread.m (calls nii2jnii)
  • niftiwrite.m (calls nifticreate)
  • niftiinfo.m (calls nii2jnii, niicodemap)
  • loadnifti.m (alias to nii2jnii)
  • savenifti.m (calls nifticreate, )
  • nifticreate.m (calls memmapstream)
  • nii2jnii.m (key - calls niiformat, memmapstream, niicodemap)
  • niicodemap.m (needs containers.Map, available in Octave 5+)
  • niiformat.m
  • memmapstream.m


both nii2jnii.m and savenifti.m need to be slightly updated to replace the gzipencode/gzipdecode calls by fopen('z') (although making the code octave-specific).

you may also place some of the needed functions into private/ folder so it won't conflict with octave-jnifti etc.

it is not my intent to propose to merge JSON/JNIfTI functionalities to octave-image (unless the package maintainer is supportive), and I am committed to maintain them as separate packages.

> I would be happy to help reformatting your code if needed.


much appreciated!

Qianqian Fang <fangq>
Mon 21 Oct 2019 04:49:59 PM UTC, comment #4: 

@Qianqian: I am not a maintainer of the image package and I am aware of the underlying context your work is built upon but I do not think the niftiread/write/info implementations in Octave should require any extra dependencies. JSON is currently nowhere in the NIfTI specifications and for compression, one could take advantage of the "z" mode for fopen() in Octave (something that Matlab does not have):
https://octave.org/doc/v5.1.0/Opening-and-Closing-Files.html
I would be happy to help reformatting your code if needed.

Guillaume <gyom>
Mon 21 Oct 2019 04:49:11 AM UTC, comment #3: 

@Hartmut, thanks for reviewing this submission.

I want to mention that I recently packaged most of the related packages for Fedora, the octave-jnifti and octave-zmat are both available for Fedora 29-32, and octave-jsonlab has been available since 2 years ago. here is my package list

https://fedoraproject.org/wiki/User:Fangq#New_packages_in_progress

regarding your questions, please see my replies below:

----------------------------------------------------------------------------

> Are all of the contributed files written by YOURSELF? And could you "donate" them with a GPL compatible license? (I have seen an Apache license in nii2jnii.m for example, is it GPL compatible? Or could you re-license it under GPL?)



yes, I am the upstream author for the mentioned toolboxes. I confirm that the licenses of these toolboxes are all either GPL or GPL compatible (including dual-license in GPL compatible licenses).

Apache 2.0 and BSD licenses are both GPL compatible

https://fedoraproject.org/wiki/User:Fangq#New_packages_in_progress

----------------------------------------------------------------------------

> Could you list the necessary LIBRARIES (toolboxes) that your code depends on in more detail? I have already understood the following dependencies:
>
>JNIfTI toolbox
>ZMat toolbox (for dealing with compression?)
>JSONLab toolbox (for dealing with Json?)
>
> any more tolboxes or library necessary?


no

----------------------------------------------------------------------------

> Are you the author of all of the above toolboxes? Are those all available under a GPL compatible license (I have seen GPLv3 at many places in the mentioned toolboxes)



Some of the dependent toolboxes contain small parts from other authors, but these units all have licenses that are compatible with the licenses of the corresponding packages, I documented all the included 3rd party units in the README files

ZMat: https://github.com/fangq/zmat/blob/master/README.rst#acknowledgement
JSONLab: https://github.com/fangq/jsonlab/blob/master/README.rst#acknowledgement


----------------------------------------------------------------------------

> The octave image toolbox is currently "self contained" in the sense that it does not require to install any formal dependencies beforehand. So all code that your new functions need from the mentioned toolboxes would probably also need to be included into the Octave image toolbox. This is the reason behind the next question:
>
> Would we need to include ALL CODE of the above toolboxes?


it depends on to what extend you want to support nifti files - uncompressed or compressed nifti. If just uncompressed nifti, the only dependency you need is the jnifti toolbox; if you need to read/write nii.gz/hdr.gz/img.gz files, which are more popular among users, you will either need to call gunzip to uncompress the file on the disk, and then read using the jnifti toolbox; but calling zmat is more convenient as it does not need involving reading/writing additional files.

as I mentioned above, I started packaging these toolboxes for Fedora, and will be working next for Debian/Ubuntu. I am happy to adjust the content/dependencies of my packages to accommodate whatever that you would like to be included in octave-image, and what you want to leave in separate packages. I also do not think a small overlap in these toolboxes is a problem.

----------------------------------------------------------------------------

> Or is some of the functionality already implemented in Octave and can be used instead? (Octave can already deal with compressed files I think. Is this functionality not good enough for your intended use?)


as mentioned above, you can call gzip/gunzip to compress/uncompress the files before processing, but it is not as convenient as zmat and requires disk-write permission to proceed.

----------------------------------------------------------------------------

> Are some functions of those toolboxes NOT used by your nitfi functions and could therefore be omitted from the inclusion into the Octave image package?


if you want to keep the number of units as little as possible, you may remove jnifticreate.m, loadjnifti.m, savejnifti.m, savejnii.m and savebnii.m. These are for processing JSON-represented nifti (i.e. jnifti) data files. I need to run a test and see if removing these will cause any issues.

----------------------------------------------------------------------------

> Another idea: In the case that (a) also those libraries are your own code and (b) they can be licensed with GPL and (c) they add functionality regarding compression and/or json functionality to Octave that isn't there yes, it might be worth discussing with "core Octave" about including these parts of your code there. But in this case those libraries should probably mimic Matlab compatible behavior (e.g. Matlab's jsondecode.m ...). This might be more than you intended to do.


I know someone is working on jsonencode/jsondecode using C based parsers. I left some comments in the below thread

https://savannah.gnu.org/bugs/?53100

----------------------------------------------------------------------------

> In some toolboxes there seem to be pre-compiled functions (I have seen zipmat.mex for exmaple). Are those files
> also available as C source code (under GPL)? Are they coded in a way that Octave can compile them on its own?


yes. also see my packages for Fedora - including the mex file (octave-mex), and the library (zmat, zmat-devel, zmat-static). You may use it easily to create an oct based unit.

----------------------------------------------------------------------------

> In general there is a coding style for "core Octave", regarding C-code and m-code as well. It can be found here:
> https://wiki.octave.org/Octave_style_guide
> https://wiki.octave.org/C%2B%2B_style_guide



in the current version, my code was not specifically formatted for Octave, because these toolboxes I created were shared for both MATLAB and Octave. I am happy to reformat if this is required/necessary, but I generally do not prefer to maintain two versions.

Qianqian Fang <fangq>
Sun 20 Oct 2019 06:13:39 PM UTC, comment #2: 

Thanks you for the intention to contribute code to the Octave image package. You are very welcome as a first time contributor!

Those nifti-xxx functions were introduced to the Matlab image processing toolbox with version R2017b. So in general it would be very nice to have them in Octave as well.

I have personally never used a nifti file so far, nor am I an expert in software development, including the libraries you mentioned. (So some of the following questions might not sound very clever. I do know something about image processing, though...)

Here are some questions that come to my  mind:

  • Are all of the contributed files written by YOURSELF? And could you "donate" them with a GPL compatible license? (I have seen an Apache license in nii2jnii.m for example, is it GPL compatible? Or could you re-license it under GPL?)


  • Could you list the necessary LIBRARIES (toolboxes) that your code depends on in more detail? I have already understood the following dependencies:
    • JNIfTI toolbox
    • ZMat toolbox (for dealing with compression?)
    • JSONLab toolbox (for dealing with Json?)
    • any more tolboxes or library necessary?


  • Are you the author of all of the above toolboxes? Are those all available under a GPL compatible license (I have seen GPLv3 at many places in the mentioned toolboxes)


  • The octave image toolbox is currently "self contained" in the sense that it does not require to install any formal dependencies beforehand. So all code that your new functions need from the mentioned toolboxes would probably also need to be included into the Octave image toolbox. This is the reason behind the next question:
  • Would we need to include ALL CODE of the above toolboxes?
    • Or is some of the functionality already implemented in Octave and can be used instead? (Octave can already deal with compressed files I think. Is this functionality not good enough for your intended use?)
    • Are some functions of those toolboxes NOT used by your nitfi functions and could therefore be omitted from the inclusion into the Octave image package?


  • Another idea: In the case that (a) also those libraries are your own code and (b) they can be licensed with GPL and (c) they add functionality regarding compession and/or json functionality to Octave that isn't there yes, it might be worth discussing with "core Octave" about including these parts of your code there. But in this case those libraries should probably mimic Matlab compatible behavior (e.g. Matlab's jsondecode.m ...). This might be more than you intended to do.


  • In some toolboxes there seem to be pre-compiled functions (I have seen zipmat.mex for exmaple). Are those files also available as C source code (under GPL)? Are they coded in a way that Octave can compile them on its own?



Please regard my above questions as expression of my interest into your code.

Maybe someone with more knowledge about C-code and library inclusion policies in core Octave could also leave some comments here?

@Carne: Could you comment on this, maybe?

@Qianqian: If you don't get a response on this bug tracker during the next weeks, it might be worht leaving a short note (mostly pointing to this bug tracker issue) on the mailinglist: -email is unavailable- .




Hartmut <hardy>
Sat 19 Oct 2019 07:28:12 PM UTC, comment #1: 

adding people to cc

Hartmut <hardy>
Fri 13 Sep 2019 04:52:26 AM UTC, original submission:  

I would like to contribute the below missing functions to octave-image

- niftiread
- niftiinfo
- niftiwrite

The current version can be found at
https://github.com/fangq/jnifti/tree/master/lib/octave

and are also attached in this thread. They can handle .nii, .nii.gz, .hdr/.img, .hdr.gz/.img.gz files. The input/output of these function smatch MATLAB's versions.

The newly drafted functions do require a set of helper functions to fully function:

- To read/write un-compressed NIfTI-1/2 (.nii) files, it minimally needs the JNIfTI toolbox at https://github.com/fangq/jnifti/tree/master/lib/matlab

- To read/write gzip-compressed NIfTI-1/2, it needs the ZMat toolbox at http://github.com/fangq/zmat

- To convert the NIfTI data to the more portable version for easy manipulation, i.e. the JNIfTI format, it needs the JSONLab toolbox at http://github.com/fangq/jsonlab

I've tested these functions on both Octave 4.2 and 5.1, using the NIfTI-1/2 sample data provided at

https://nifti.nimh.nih.gov/nifti-1/data

all works nicely.

I am wondering if anyone is interested in reviewing these functions and give some suggestions? first time contributor to Octave.

Qianqian Fang <fangq>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #47502:  niicodemap.m added by fangq (3KiB - application/octet-stream)
file #47503:  savebnii.m added by fangq (2KiB - application/octet-stream)
file #47504:  savejnifti.m added by fangq (2KiB - application/octet-stream)
file #47505:  savejnii.m added by fangq (2KiB - application/octet-stream)
file #47498:  nifticreate.m added by fangq (1KiB - application/octet-stream - add more dependent functions)
file #47499:  memmapstream.m added by fangq (2KiB - application/octet-stream - add more dependent functions)
file #47500:  jnifticreate.m added by fangq (3KiB - application/octet-stream - add more dependent functions)
file #47501:  loadjnifti.m added by fangq (2KiB - application/octet-stream - add more dependent functions)
file #47494:  niftiinfo.m added by fangq (4KiB - application/octet-stream - Need more functions from https://github.com/fangq/jnifti/tree/master/lib/matlab)
file #47495:  niftiread.m added by fangq (2KiB - application/octet-stream - Need more functions from https://github.com/fangq/jnifti/tree/master/lib/matlab)
file #47496:  niftiwrite.m added by fangq (2KiB - application/octet-stream - Need more functions from https://github.com/fangq/jnifti/tree/master/lib/matlab)
file #47497:  nii2jnii.m added by fangq (13KiB - application/octet-stream - Need more functions from https://github.com/fangq/jnifti/tree/master/lib/matlab)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by hardy
  • -email is unavailable- added by hardy
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by fangq (Submitted the item)
  • -email is unavailable- added by fangq
  • -email is unavailable- added by fangq
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 18 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-01-28 mtmiller Summary(image) implement niftiread, niftiwrite, niftiinfo [octave forge] (image) implement niftiread, niftiwrite, niftiinfo
    2019-10-19 hardy Carbon-Copy- Added avinoam
        Carbon-Copy- Added carandraug
    2019-10-01 avinoam Carbon-Copy- Added hardy
    2019-09-13 fangq Attached File- Added niicodemap.m, #47502
        Attached File- Added savebnii.m, #47503
        Attached File- Added savejnifti.m, #47504
        Attached File- Added savejnii.m, #47505
    2019-09-13 fangq Attached File- Added nifticreate.m, #47498
        Attached File- Added memmapstream.m, #47499
        Attached File- Added jnifticreate.m, #47500
        Attached File- Added loadjnifti.m, #47501
    2019-09-13 fangq Attached File- Added niftiinfo.m, #47494
        Attached File- Added niftiread.m, #47495
        Attached File- Added niftiwrite.m, #47496
        Attached File- Added nii2jnii.m, #47497
        Carbon-Copy- Added carandraug
        Carbon-Copy- Added gyom

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code