WIP: CIFTI2 spec reader and writer #249

satra · 2014-08-01T16:41:50Z

CIFTI2 reader
CIFTI2 writer
tests
integration with nifti2

satra · 2014-08-01T16:42:44Z

@matthew-brett - a quick pass at reading a cifti2 spec based on the gifti reader. i'll try to finish this up over the weekend.

coveralls · 2014-08-01T16:45:29Z

Coverage decreased (-2.44%) when pulling e80b4bc on satra:enh/cifti2 into f8eb71d on nipy:master.

coveralls · 2014-08-02T20:50:00Z

Coverage decreased (-2.97%) when pulling 2262b73 on satra:enh/cifti2 into f8eb71d on nipy:master.

satra · 2014-08-03T03:23:48Z

@matthew-brett - have you read the cifti2 spec?

one question is whether we want to create cifti data type specific models (dconn, dtseries, dscalar, etc.,.). if this is the case, we will need to focus on these data types and how the cifti extension is manipulated based on the data type. (unfortunately that will require a fair bit of work). on the other hand, if we simply provide access to the cifti metadata and allow people to construct a cifti extension, that's less work.

matthew-brett · 2014-08-03T08:09:53Z

I haven't read the spec, no.

In ignorance, it sounds like an API to construct the relevant information, with a good test suite, would be the best first step. Does that seem reasonable?

I got an email of a comment on this thread with a question about the nifti2 data shape being out of spec - did you edit that out?

satra · 2014-08-03T14:45:45Z

@matthew-brett - the current implementation provides that simple api, i'll finish off the test suite. but this really should be redone with a better api.

yup edited that out - was an erroneous question.

…ror message

satra · 2014-08-04T17:28:23Z

nibabel/cifti/cifti.py

+    seriesExponent = int
+    seriesStart = float
+    seriesStep = float
+    seriesUnit = str


@matthew-brett - since i initially built this off the gifti classes i followed the standards there. i usually don't declare class variables up front. is this something that nibabel follows or intended to help docs/py3k?

No - I don't know where that habit came from, but I'm with you, and I don't declare variables up front. I think you'll see very few of the other nibabel classes use this.

satra · 2014-08-04T17:32:27Z

@matthew-brett - i'm going to now ask for comments/feedback on both the nipy and hcp mailings lists.

completed thus far:

basic io (not much validation or checking - for example, all the asserts should really raise exceptions)
basic tests (can i read, write, and read back what i write)
base CiftiImage class
- we should be deriving datatype specific classes from this and adding api to those specific data types (scalars, timeseries, fibers, etc.,.)

coveralls · 2014-08-04T17:34:17Z

Coverage decreased (-0.04%) when pulling 8b0feb6 on satra:enh/cifti2 into f8eb71d on nipy:master.

matthew-brett · 2014-08-04T18:11:14Z

nibabel/cifti/cifti.py

+    def __init__(self, nvpair = None):
+        self.data = []
+        if not nvpair is None:
+            if isinstance(nvpair, list):


Has to be list rather than - say - tuple?

matthew-brett · 2014-08-04T18:25:29Z

nibabel/cifti/cifti.py

+        if vertices is None:
+            vertices = []
+        self.vertices = vertices
+        self.numVA = len(vertices)


numVa as property? Can you avoid non-pep-8 name? num_va?

coveralls · 2017-02-04T20:26:42Z

Coverage increased (+0.1%) to 96.138% when pulling 2fa22d1 on satra:enh/cifti2 into b8017c9 on nipy:master.

coveralls · 2017-02-04T20:46:05Z

Coverage increased (+0.1%) to 96.138% when pulling 2fa22d1 on satra:enh/cifti2 into b8017c9 on nipy:master.

demianw · 2017-02-08T16:29:15Z

The appveyor tests seem to fail due to a testing implementation problem. Is that the last thing we need to fix this?

nibotmi · 2017-02-13T14:03:46Z

☔ The latest upstream changes (presumably #508) made this pull request unmergeable. Please resolve the merge conflicts.

coveralls · 2017-02-15T14:53:35Z

Coverage increased (+0.1%) to 96.141% when pulling 72fcdc9 on satra:enh/cifti2 into 7044ee4 on nipy:master.

satra · 2017-02-15T15:09:43Z

@matthew-brett - any ideas here? it seems that appveyor is seeing some conflict with cleaning up the temporary directory for the various cifti write-read-assert tests

Windows needs to have all objects that carry file references to be deleted, before deleting the referenced files.

matthew-brett · 2017-02-16T03:21:36Z

Satra - see satra#17 - Windows needs all objects deleted that carry open file objects, before it can delete the referenced files.

There seem to be quite a few pep8 problems in the test_new_cifti2.py file.

BF: delete images to allow Windows to delete dir

satra · 2017-02-16T03:23:53Z

thanks @matthew-brett - i'll do a pep8 sweep through that file.

coveralls · 2017-02-16T03:46:02Z

Coverage increased (+0.1%) to 96.151% when pulling ab1801f on satra:enh/cifti2 into 7044ee4 on nipy:master.

effigies · 2017-02-17T01:38:32Z

Looks finally ready to merge? Or are there PEP8 failures that are being suppressed by tox.ini that need addressing? (I've never been entirely clear on the style rules for tests.)

coveralls · 2017-02-17T04:43:31Z

Coverage increased (+0.1%) to 96.151% when pulling e4f3176 on satra:enh/cifti2 into 7044ee4 on nipy:master.

satra · 2017-02-17T14:19:27Z

@matthew-brett - pep8 for test_new_cifti2 done. is there anything else that needs to happen before merge?

coveralls · 2017-02-17T14:39:11Z

Coverage decreased (-0.007%) to 96.005% when pulling 9e1de24 on satra:enh/cifti2 into 7044ee4 on nipy:master.

matthew-brett · 2017-02-26T03:54:03Z

Sorry to be slow to get back to you - traveling and being sick...

I won't have time to go through this guy in detail now, so what I propose to do is merge this as is, and the do some pull requests with proposed edits and refactorings. That means that the interface may change after it gets merged. Is this OK with everyone?

demianw · 2017-02-26T07:52:23Z

I am ok with this

satra · 2017-02-26T13:26:16Z

@matthew-brett - ok with me.

matthew-brett · 2017-03-07T02:10:50Z

Sorry for the delay - thanks for all the work. In it goes. I'll try to get to some refactoring this week.

satra · 2017-03-07T02:23:29Z

@matthew-brett - thank you!!

demianw · 2017-03-07T07:04:28Z

Yay!

effigies · 2017-03-07T12:43:47Z

@matthew-brett I'll be happy to review your PRs. Since so many people are depending on Satra's branch, I think it'd be good to aim for a release soon so they can depend on a stable version.

demianw · 2017-03-07T13:15:49Z

@effigies I think that now that this has been merged we can start PRs to build convenience functions for navigating the data and creating CIFTI2 images

satra · 2017-03-07T13:32:14Z

@demianw - corresponding to the tests that start here written by @MichielCottaar : https://github.com/nipy/nibabel/blob/master/nibabel/cifti2/tests/test_new_cifti2.py#L206 there are few of these convenience functions already at the top of the file, which can be extracted and added to the library.

satra added 2 commits August 1, 2014 12:39

enh: first pass at the cifti2 reader

6fa78bc

fix: remove cifti element spec

e80b4bc

enh: cifti writer enabled

2262b73

satra added 6 commits August 4, 2014 00:03

ref: initial refactor towards cifti datatypes

db0d2af

fix: updated to reflect latest CIFTI-2 spec and nifti extensions

c862285

tst: added test data

645edaa

fix: voxelindicesijk initialization, header version check, typo in er…

3d19b50

…ror message

fix: version number for looseversion

e1ab596

tst: added basic io tests

8b0feb6

satra reviewed Aug 4, 2014
View reviewed changes

matthew-brett reviewed Aug 4, 2014
View reviewed changes

fix: removed redundant file writing

75a2906

matthew-brett reviewed Aug 4, 2014
View reviewed changes

resolved conflict

72fcdc9

BF: delete images to allow Windows to delete dir

4b389e9

Windows needs to have all objects that carry file references to be deleted, before deleting the referenced files.

Merge pull request #17 from matthew-brett/cifti-fix

ab1801f

BF: delete images to allow Windows to delete dir

PEP8: test file

e4f3176

ref: if formulation

9e1de24

matthew-brett merged commit 19fd3f8 into nipy:master Mar 7, 2017

nibotmi mentioned this pull request Mar 7, 2017

Support for BrainVoyager VTC, MSK, VMR and NR-VMP files #216

Open

demianw mentioned this pull request Mar 24, 2017

Problem loading large CIFTI2 files in lastest updates #520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: CIFTI2 spec reader and writer #249

WIP: CIFTI2 spec reader and writer #249

satra commented Aug 1, 2014

satra commented Aug 1, 2014

coveralls commented Aug 1, 2014

coveralls commented Aug 2, 2014

satra commented Aug 3, 2014

matthew-brett commented Aug 3, 2014

satra commented Aug 3, 2014

satra Aug 4, 2014

matthew-brett Aug 4, 2014

satra commented Aug 4, 2014

coveralls commented Aug 4, 2014

matthew-brett Aug 4, 2014

matthew-brett Aug 4, 2014

coveralls commented Feb 4, 2017

coveralls commented Feb 4, 2017

demianw commented Feb 8, 2017

nibotmi commented Feb 13, 2017

coveralls commented Feb 15, 2017

satra commented Feb 15, 2017

matthew-brett commented Feb 16, 2017

satra commented Feb 16, 2017

coveralls commented Feb 16, 2017

effigies commented Feb 17, 2017

coveralls commented Feb 17, 2017

satra commented Feb 17, 2017

coveralls commented Feb 17, 2017 •

edited

Loading

matthew-brett commented Feb 26, 2017

demianw commented Feb 26, 2017

satra commented Feb 26, 2017

matthew-brett commented Mar 7, 2017

satra commented Mar 7, 2017

demianw commented Mar 7, 2017

effigies commented Mar 7, 2017

demianw commented Mar 7, 2017

satra commented Mar 7, 2017

WIP: CIFTI2 spec reader and writer #249

WIP: CIFTI2 spec reader and writer #249

Conversation

satra commented Aug 1, 2014

satra commented Aug 1, 2014

coveralls commented Aug 1, 2014

coveralls commented Aug 2, 2014

satra commented Aug 3, 2014

matthew-brett commented Aug 3, 2014

satra commented Aug 3, 2014

satra Aug 4, 2014

Choose a reason for hiding this comment

matthew-brett Aug 4, 2014

Choose a reason for hiding this comment

satra commented Aug 4, 2014

coveralls commented Aug 4, 2014

matthew-brett Aug 4, 2014

Choose a reason for hiding this comment

matthew-brett Aug 4, 2014

Choose a reason for hiding this comment

coveralls commented Feb 4, 2017

coveralls commented Feb 4, 2017

demianw commented Feb 8, 2017

nibotmi commented Feb 13, 2017

coveralls commented Feb 15, 2017

satra commented Feb 15, 2017

matthew-brett commented Feb 16, 2017

satra commented Feb 16, 2017

coveralls commented Feb 16, 2017

effigies commented Feb 17, 2017

coveralls commented Feb 17, 2017

satra commented Feb 17, 2017

coveralls commented Feb 17, 2017 • edited Loading

matthew-brett commented Feb 26, 2017

demianw commented Feb 26, 2017

satra commented Feb 26, 2017

matthew-brett commented Mar 7, 2017

satra commented Mar 7, 2017

demianw commented Mar 7, 2017

effigies commented Mar 7, 2017

demianw commented Mar 7, 2017

satra commented Mar 7, 2017

coveralls commented Feb 17, 2017 •

edited

Loading