RF: Use python properties, rather than set/get methods #353
Conversation
|
Sorry to say, but yes, we have advertised the GIFTI support, and people have used it, so I think we do have to deprecate the get_ / set_ methods... |
|
OK. Is it worth it to make this change, in this case? Happy to deprecate & test, but also OK to toss. I'm just working through the code on my way to load/save GIFTI. |
|
If you have the heart to do the deprecations, I think it's an improvement... |
|
I sure do! Will get to that tonight or tomorrow, then :) |
|
Added the get/set deprecations and deprecation tests. |
| @@ -427,14 +425,12 @@ def add_gifti_data_array(self, dataarr): | |||
| """ | |||
| if isinstance(dataarr, GiftiDataArray): | |||
| self.darrays.append(dataarr) | |||
| self.numDA += 1 | |||
There was a problem hiding this comment.
numDA being tested properly already?
There was a problem hiding this comment.
Not sure about tests, will check.
This line was removed as I made numDA a computed read-only property.
There was a problem hiding this comment.
Yes, sorry to be confusing - I saw you didn't need the line, but I was wondering if there were tests to make sure the computation was working right.
There was a problem hiding this comment.
There were not; I just added some.
|
I updated some other functions that are better as properties and added basic tests. Please take another look when possible! |
| else: | ||
| print("Not a valid GiftiLabelTable instance") | ||
| if not isinstance(labeltable, GiftiLabelTable): | ||
| raise ValueError("Not a valid GiftiLabelTable instance") |
There was a problem hiding this comment.
Note the change from print to ValueError here.
bcipolli
changed the title
| self.red = rgba[0] | ||
| self.green = rgba[1] | ||
| self.blue = rgba[2] | ||
| self.alpha = rgba[3] |
There was a problem hiding this comment.
Personal preference, feel free to reject:
self.red, self.green, self.blue, self.alpha = rgba
|
Read-through done. Logic looks good. |
|
Updated based on last comments. |
|
|
||
| # Test default value | ||
| gl4 = GiftiLabel() | ||
| assert_is_none(gl4.rgba[0]) |
There was a problem hiding this comment.
2.6 seems upset.
assert_true(gl4.rgba[0] is None)
There was a problem hiding this comment.
That's what I get for trying to be too fancy :)
|
Okay, this LGTM. @matthew-brett The Travis build seems to be failing on unrelated things. (Happening here and on #354.) |
RF: Deprecate get/set methods in favor of Python properties Test consistency of data array counter Warn on inconsistent NumberofDataArrays and DataArray elements Remove commented code from nibabel/gifti/gifti.py
RF: Deprecate get/set methods in favor of Python properties Test consistency of data array counter Warn on inconsistent NumberofDataArrays and DataArray elements Remove commented code from nibabel/gifti/gifti.py
numDA,version, andfilenameproperties on GIFTI haveset_andget_methods. Seems cleaner (and more pythonic) to use Python properties instead.Question: is GIFTI support officially published? I.e. do I need to add deprecation warnings for the old methods, and deprecation tests?
Also renamed tests to be clear that they're testing the loading of these properties, not necessarily of their setting or saving.