More GIFTI fixes #403

bcipolli · 2016-02-07T17:42:46Z

This is an old branch that I never got to submitting. More commits to make GIFTI more pythonic and test the functionality a bit more thoroughly.

effigies · 2016-02-07T20:17:46Z

nibabel/gifti/tests/test_gifti.py

@@ -11,8 +11,7 @@
 from nibabel.gifti.gifti import data_tag
 from nibabel.nifti1 import data_type_codes, intent_codes

-from numpy.testing import (assert_array_almost_equal,
-                           assert_array_equal)
+from numpy.testing import (assert_array_almost_equal, assert_array_equal)


Drop the parentheses.

The commit message makes bc017aa look like it must have other stuff rebased out. Maybe re-write that one to match what's left?

bcipolli · 2016-02-08T01:45:05Z

OK, I've looked this over and made tweaks so that tests pass. I'm going to ignore the 0.004% decrease in test coverage (due to decreasing the total amount of code lol); I think this is ready to go!

effigies · 2016-02-08T02:12:16Z

nibabel/gifti/parse_gifti_fast.py

                di = "Dim%s" % str(i)
                if di in attrs:
                    self.da.dims.append(int(attrs[di]))
            # dimensionality has to correspond to the number of DimX given
-            assert len(self.da.dims) == self.da.num_dim
+            # TODO (bcipolli): don't assert; raise parse warning, and recover.
+            assert len(self.da.dims) == self.num_dim


Sorry to prolong this. Unless I'm missing something, self.num_dim is never referenced outside this function. Any reason not to make it a local variable?

No problem at all, thanks for your eyes on it! Good point; it was a member variable and I didn't think hard enough to realize it could be made local now. Will take care of that!

bcipolli · 2016-02-09T05:15:46Z

@effigies I appreciate the feedback and your timely code reviews--very much so. At the same time, I feel like we're going backwards here.

First, it feels very odd to fix something, only to get new comments to change something else. I have no idea when this PR will complete because each time something change, something new is requested. I'd really much rather get all the feedback at once, then get one request every time I change something.

Second, more and more I feel your comments are expanding the scope of the original PR. This PR is an improvement on what exists, not to scrub clean any code that I touch. The more the scope expands, the less interested I am to complete the PR or, to be frank, to contribute at all.

I agree that Gifti (and for that matter, many Nibabel objects) could use better documentation. I disagree that this needs to happen in this PR. I have much higher priorities with nibabel than writing docstrings for Gifti, and going through these intensive code reviews is actually preventing me from getting there.

So for me, either we need to make it easier to contribute code to nibabel, or I need to solve my problems without contributing back the code. But I can't spend this much of my time working and reworking and reworking existing code. I appreciate your and @matthew-brett 's willingness to use your time in that way, but I really don't have the time for it any longer, and need to get to the research quicker than what' been happening here.

Thanks!

matthew-brett · 2016-02-09T05:46:35Z

HI Ben - it sounds like we could do a better job of helping you here.

Could you give us an idea of how we could do that? I hear your feeling that progress is too slow, and I agree, for which I am clearly to blame. I guess your frustration is partly the fact that I'm sometimes being responsive, and sometimes not, therefore slowing you down. So, I'll try very hard to keep right on top of stuff this week. Expect me to respond within a couple of hours (I have a few meetings this week). Also, please feel free to hit me on gchat if you want faster feedback.

But for specifics, I think it's completely reasonable for us not to ask you to clean code you haven't touched yourself, and please just say when we're doing that and you don't think it's reasonable.

Also please feel free to say when you'd rather not make a change, that's a reasonable answer, reviewing, as you know, is hard task, and we need feedback to do it properly.

Let's get this finished up in the next couple of days.

Lastly - Chris - I'd like to add my deep thanks for all your help - I really appreciate it.

effigies · 2016-02-09T05:49:21Z

@bcipolli I appreciate where you're coming from. So you understand my position: I'm generally looking at these as I get a spare moment (or need to procrastinate on something) and making a comment as I see something. If you would rather that I take more time and present my thoughts all at once to avoid the back-and-forth thing, I can definitely do that.

Also, my assumption (now that the blitz of PRs from last year is over) is that @matthew-brett is the primary reviewer. This is not my project, and I feel uncomfortable okaying changes that might ultimately make things less clean. Hence the feeling-out of whether you feel like we're moving out-of-scope and deference to Matthew's opinions. I'm trying to ease the burden on both of you, because you've been doing a ton of work and he's keeping this whole thing afloat, but if I'm actually just annoying, I can step back.

So maybe in the short term (this PR), I'd recommend you take my comments as suggestions at best and sync on Matthew's time-scale. In the long-term, I hope that we can settle on a non-alienating review process.

matthew-brett · 2016-02-09T06:23:23Z

On a little reflection - two thoughts:

Maybe it would be useful to have a 20 minute hangout tomorrow sometime. Are you both around? I'm free anytime except 4 til 6 pacific time.
If we're getting stuck, one option is to merge this and follow up with pull-requests to the merged code.

Chris - sorry you got stuck with me off doing other stuff and not paying proper attention - that's a tough position to be in, and thanks for being patient.

effigies · 2016-02-09T13:54:53Z

Hangouts in general are hard for me, as I work in a pretty open lab where I'd be bothering people. If you set up a time that works for you two, I can see if I can make it work. Otherwise I'm happy to defer to your decisions.

No objections to merging this PR as is.

matthew-brett · 2016-02-09T23:28:19Z

Ben - what do you think?

bcipolli · 2016-02-10T02:30:57Z

Sorry--limited time these days, juggling a lot of things, hence being more strict about my time.

The hard thing for me is budgeting my time. @effigies perhaps if you're working through a PR piecemeal, just indicate so. I would simply lay off making any changes until you finish, so I can run through them more efficiently.

I'd also suggest that, if all of our time is so constrained, we might want to loosen up the code reviews. They're time-consuming for all involved, and I feel they're a major bottleneck. I'd rather write more automated tests than spend so much time iterating on style... but that's just my feeling.

@matthew-brett Sorry for not catching your suggestion for the hangout earlier. I'm at a conference in DC, and juggling a few things in addition, so ... hard for me to make these days. I'll be home Thursday/Friday if y'all want to chat.

My goal is, and always has been, to get CIFTI support in so that I can work with HCP data in Python. Hope we can get there soon!

matthew-brett · 2016-02-10T04:50:01Z

Yes, I think we can get there soon. And this is a good moment to take stock of the review process and try and improve it.

What would you like me to do? I am free tomorrow, and I will be reviewing another huge nibabel pull request (#391), but I can also go over the cifti one. That way you'd have all or nearly all pertinent comments for Thursday.

The alternatives are:

merge as is and me / Chris submit PRs for you to review;
wait until Thursday / Friday, chat about it on a hangout and proceed from there, planning to get the work merged after a couple of days.

bcipolli · 2016-02-10T05:53:51Z

I think, just proceed as usual. Knowing that things are coming in pieces helps, and knowing that I can suggest to not implement some of the feedback helps too. I will just let things sit and wait until I hear the magic words--"Finished the review, and ... " :)

matthew-brett · 2016-02-11T01:34:05Z

OK, fair enough. I should have finished the review by end of the day tomorrow.

matthew-brett · 2016-02-12T02:03:25Z

nibabel/gifti/gifti.py

@@ -104,6 +104,7 @@ def print_summary(self):


 class GiftiLabel(xml.XmlSerializable):
+    """


Ben - feel free to leave these, I'll submit another PR with docstring edits.

matthew-brett · 2016-02-12T02:15:46Z

All finished from my PoV. Thanks for being patient. I'll email to the neuroimaging mailing list about ways of my being more responsive. Please hold me to strictly a maximum two day delay on PR reviews for your stuff, especially for small changes like these.

…ray.

…tance-level variables.

bcipolli · 2016-02-12T16:02:49Z

@effigies @matthew-brett Thanks for the reviews. I added back a string test, left docstrings as-is, and rebased on the latest master. I think that should do it?

matthew-brett · 2016-02-13T04:38:33Z

Fine with me. Chris - any comments? Otherwise I plan to merge tomorrow.

effigies · 2016-02-13T04:41:22Z

Nope, I'm good.

On February 12, 2016 11:38:34 PM EST, Matthew Brett [email protected] wrote:

Fine with me. Chris - any comments? Otherwise I plan to merge
tomorrow.

Reply to this email directly or view it on GitHub:
#403 (comment)

Chris Markiewicz

matthew-brett · 2016-02-13T05:36:21Z

OK - thanks to you both - merging.

MRG: More GIFTI fixes More commits to make GIFTI more pythonic and test the functionality a bit more thoroughly.

Responding to comments on Ben's previous PR nipy#403. Expand and fix docstrings and gifti attribute descriptions from gifti spec. Rewrite GiftiDataArray init to allow full initialization of object, that can be used from GiftiDataArray.from_array. The refactor of the init needs tests; I'll add these if y'all agree this init refactoring is the right thing to do.

…cstrings MRG: rewrite gifti docstrings; refactor init Responding to comments on Ben's previous PR #403. Expand and fix docstrings and gifti attribute descriptions from gifti spec. Rewrite `GiftiDataArray` init to allow full initialization of object, that can be used from `GiftiDataArray.from_array`. Deprecate `GiftiDataArray.from_array` as redundant. Add tests. Fix up the tests - there were some duplicate test names so some tests were not being run. Remove unused `label` attribute of `GiftiLabel` object.

MRG: More GIFTI fixes More commits to make GIFTI more pythonic and test the functionality a bit more thoroughly.

Responding to comments on Ben's previous PR nipy#403. Expand and fix docstrings and gifti attribute descriptions from gifti spec. Rewrite GiftiDataArray init to allow full initialization of object, that can be used from GiftiDataArray.from_array. The refactor of the init needs tests; I'll add these if y'all agree this init refactoring is the right thing to do.

…d-docstrings MRG: rewrite gifti docstrings; refactor init Responding to comments on Ben's previous PR nipy#403. Expand and fix docstrings and gifti attribute descriptions from gifti spec. Rewrite `GiftiDataArray` init to allow full initialization of object, that can be used from `GiftiDataArray.from_array`. Deprecate `GiftiDataArray.from_array` as redundant. Add tests. Fix up the tests - there were some duplicate test names so some tests were not being run. Remove unused `label` attribute of `GiftiLabel` object.

effigies reviewed Feb 7, 2016
View reviewed changes

effigies reviewed Feb 8, 2016
View reviewed changes

matthew-brett reviewed Feb 12, 2016
View reviewed changes

Ben Cipollini added 6 commits February 12, 2016 07:47

deprecation tests should actually test for warnings.

255b751

data arrays created via constructor are malformed; create via from_ar…

b9fbbe0

…ray.

Do not declare class variables as types; they mask failure to set ins…

5fef80d

…tance-level variables.

make num_dim a read-only property

23dc6c1

STY: fix up pep8 failures.

78deca4

TST: add gifti string and MetaData tests.

ae094ac

matthew-brett added a commit that referenced this pull request Feb 13, 2016

Merge pull request #403 from bcipolli/gifti-fix4

15a7dd8

MRG: More GIFTI fixes More commits to make GIFTI more pythonic and test the functionality a bit more thoroughly.

matthew-brett merged commit 15a7dd8 into nipy:master Feb 13, 2016

matthew-brett mentioned this pull request Feb 13, 2016

MRG: rewrite gifti docstrings; refactor init #407

Merged

grlee77 pushed a commit to grlee77/nibabel that referenced this pull request Mar 15, 2016

Merge pull request nipy#403 from bcipolli/gifti-fix4

e514c9c

MRG: More GIFTI fixes More commits to make GIFTI more pythonic and test the functionality a bit more thoroughly.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More GIFTI fixes #403

More GIFTI fixes #403

bcipolli commented Feb 7, 2016

effigies Feb 7, 2016

bcipolli commented Feb 8, 2016

effigies Feb 8, 2016

bcipolli Feb 8, 2016

bcipolli commented Feb 9, 2016

matthew-brett commented Feb 9, 2016

effigies commented Feb 9, 2016

matthew-brett commented Feb 9, 2016

effigies commented Feb 9, 2016

matthew-brett commented Feb 9, 2016

bcipolli commented Feb 10, 2016

matthew-brett commented Feb 10, 2016

bcipolli commented Feb 10, 2016

matthew-brett commented Feb 11, 2016

matthew-brett Feb 12, 2016

matthew-brett commented Feb 12, 2016

bcipolli commented Feb 12, 2016

matthew-brett commented Feb 13, 2016

effigies commented Feb 13, 2016

matthew-brett commented Feb 13, 2016

		@@ -104,6 +104,7 @@ def print_summary(self):


		class GiftiLabel(xml.XmlSerializable):
		"""

More GIFTI fixes #403

More GIFTI fixes #403

Conversation

bcipolli commented Feb 7, 2016

effigies Feb 7, 2016

Choose a reason for hiding this comment

bcipolli commented Feb 8, 2016

effigies Feb 8, 2016

Choose a reason for hiding this comment

bcipolli Feb 8, 2016

Choose a reason for hiding this comment

bcipolli commented Feb 9, 2016

matthew-brett commented Feb 9, 2016

effigies commented Feb 9, 2016

matthew-brett commented Feb 9, 2016

effigies commented Feb 9, 2016

matthew-brett commented Feb 9, 2016

bcipolli commented Feb 10, 2016

matthew-brett commented Feb 10, 2016

bcipolli commented Feb 10, 2016

matthew-brett commented Feb 11, 2016

matthew-brett Feb 12, 2016

Choose a reason for hiding this comment

matthew-brett commented Feb 12, 2016

bcipolli commented Feb 12, 2016

matthew-brett commented Feb 13, 2016

effigies commented Feb 13, 2016

matthew-brett commented Feb 13, 2016