BF+TST: fix XML output from in-memory Gifti array #470
Conversation
GiftiDataArrays created in memory (rather than loaded from disk) had ExternalFileOffset attribute value as an integer, but for valid XML, the values have to be strings. Closes nipygh-469.
matthew-brett
force-pushed
the
gifti-serialization-bug
branch
from
5e6af2f to
5f632fe
Compare
Current coverage is 94.26% (diff: 100%)@@ master #470 diff @@
==========================================
Files 160 160
Lines 21197 21261 +64
Methods 0 0
Messages 0 0
Branches 2266 2272 +6
==========================================
+ Hits 19975 20041 +66
+ Misses 802 801 -1
+ Partials 420 419 -1
|
|
FWIW confirming that it does resolve #469 for PyMVPA. Thanks! |
|
Any comments here? |
|
Definitely fixes the bug, but given that Maybe we should have a test to check types on parsed files? |
|
Chris - yes, I think you're right. Any ideas for a good general (or not general) way to check loaded attribute types? |
|
Simplest thing I can think of (in about 30 seconds) that isn't too class-specific is something on the order of: default = klass()
loaded = klass.read_from_file(fname)
for attr in dir(default):
assert isinstance(getattr(loaded, attr), type(getattr(default, attr)))Can probably imagine some ways for this to fail, but the other options I can think of would be a lot of work. |
why it shouldn't match type exactly but allow for subclassing? Yaroslav O. Halchenko |
|
Chris - looks like a reasonable check to me - any interest in a quick PR? |
|
PR against your branch, Matthew. It'll fail the Python 2 checks as noted over there. |
Add / use helper function for converting string to integer, where empty string maps to 0.
Specify all default GIFTI strings as unicode.
| continue | ||
| loadedtype = type(getattr(loaded, attr)) | ||
| assert_equal(loadedtype, defaulttype, | ||
| "Type mismatch for attribute: {} ({!s} != {!s})".format( |
There was a problem hiding this comment.
Sorry, forgot about 2.6:
"Type mismatch for attribute: {0} ({1!s} != {2!s})".format(
There was a problem hiding this comment.
Actually, now I'm trying to debug the errors on Python 2.6, I think it's time for 2.6 to go. It's getting really hard to set up testing on 2.6. OK with you?
Goodbye old friend. Thanks for all the fish.
Note where requirements should be updated. Update minimum numpy version. Note new minimum Python version (2.7).
|
Ok - thanks for the feedback - it's certainly a better PR now. Any more comments? |
|
LGTM. |
| # Check these against | ||
| # nibabel/info.py | ||
| # .travis.yml | ||
| # doc/source/installation.rst |
There was a problem hiding this comment.
Btw, what do you think about specifying minimal versioned requirements in setup.py, and then allow for structured optional dependencies (see eg how we have it in datalad)? See eg https://caremad.io/2013/07/setup-vs-requirement/ for the point
There was a problem hiding this comment.
Not for this PR but just saw these notes inspired by duplication, so decided to ask ;-)
|
OK to merge this one? |
|
👍 |
GiftiDataArrays created in memory (rather than loaded from disk) had
ExternalFileOffset attribute value as an integer, but for valid XML, the
values have to be strings.
Closes gh-469.