RF: Use XmlSerializable, XmlParser for Gifti load/save #365

bcipolli · 2015-10-23T17:48:33Z

CiftiImage (#249) and GiftiImage share a lot of logic. Both have functions for parsing, both will use similar logic for loading and saving. XmlBasedImage and XmlImageParser encapsulates the shared code, and defines a (hopefully) clean object model for any future images that are defined in XML.

XmlImageParser - a new abstract class that does most of the work here. Defines a structure for parsing XML files; every xml-based image will have to define one, specific to that image format. This essentially generalizes the object model used in parse_gifti_fast.py.
XmlBasedImage - a light wrapper around FileBasedImage that calls into the XmlImageParser for the image class on read, and takes care of small details on write (call into to_xml()). New required class property: parser (similar usage to header)

Things done:

Define XmlImageParser and XmlBasedImage
Migrate GiftiImage to use XmlBasedImage, and parse_gifti_fast to use XmlImageParser
Deprecate parse_gifti_file and related objects.

Things to do:

Determine the file structure; there is an unresolvable dependency between gifti.py and parse_gifti_fast.py; each refers to objects in the other. Perhaps we can simply combine them?
Write tests
Migrate parse_gifti_fast tests to new object model.
Add deprecation tests for parse_gifti_file
Add documentation
Scrub GIFTI parser code
Add test to test buffer_size passing via nibabel.load()

I'm pushing this PR out there now as (1) it seems to pass tests and (2) to get feedback on the object model / design as early in the development process as possible.

I'm not in a big rush on this; just wanted to get it out there while it was on my mind.

… and attached to each XmlBasedImage subclass.

bcipolli · 2015-10-26T14:37:03Z

nibabel/gifti/gifti.py

-class GiftiImage(FileBasedImage, xml.XmlSerializable):
+class GiftiImage(xml.XmlBasedImage):
+    """
+    The Gifti spec suggests using the following suffixes to your


Here, I moved the docstring from GiftiImage.to_file_map to GiftiImage, so that to_file_map could be moved to XmlBasedImage.

…tests)

bcipolli · 2015-10-26T22:01:22Z

Checked off all items but one: not sure the best way to avoid the recursive imports here. Should be ready for a looksie anytime--I even tried scrubbing some of the old code :)

bcipolli · 2015-10-27T15:19:54Z

@effigies I put CiftiImage on top of this code this morning. Turns out that CiftiImage is a Nifti2Image with an xml-based extension; it is not an Xml-based image. This changed two things:

CiftiImage will not subclass XmlBasedImage; only GiftiImage will.
The CiftiImage extension will need an xml parser. Using the object model here was great, but XmlImageParser is too specific a name now. I am thinking to change to XmlParser.

I think the object model and abstractions here are good; I'd like to keep them. But, since GiftiImage is the only one using XmlBasedImage, it could be considered overkill. Alternately, I could remove XmlBasedImage, move logic back to GiftiImage, keepxmlutils.py`, and simply add the parser there.

effigies · 2015-10-27T16:27:49Z

I suppose I'm inclined to not bother with XMLBasedImage if GiftiImage is its only subclass, but I should make it clear that I have not even started to look at this PR. (Busy week. I don't expect to be useful on here until Monday at the earliest.)

effigies · 2015-11-04T00:11:19Z

nibabel/gifti/parse_gifti_fast.py

+    @np.deprecate_with_doc("Use GiftiImageParser instead.")
+    def __init__(self, *args, **kwargs):
+        super(Outputter, self).__init__(*args, **kwargs)
+        self.img = None


Outputter had an initialize method that should be preserved.

effigies · 2015-11-04T01:29:44Z

Sorry for the delay. This looks reasonable to me.

The main design issue I see is still whether or not we want an XMLBasedImage with only one subclass. In favor of XMLBasedImage, the decoupling work has already been done, and the alternative is to stick generic code into GiftiImage. In opposition, now you have to read another class to understand GiftiImage.

I think I can go either way. If anybody else has strong opinions, I can probably be pushed to one side or the other.

bcipolli · 2015-11-04T17:16:19Z

I would prefer to move the code back into GiftiImage, rename XmlImageParser to XmlParser (as it's the CIFTI header that's XML-based), and move xmlimages.py back to xmlutils.py.

effigies · 2015-11-04T17:46:32Z

That sounds good to me.

bcipolli · 2015-11-04T23:04:23Z

@effigies I did as outlined above, and deprecated Outputter.initialize. Tests pass as well.

effigies · 2015-11-04T23:17:25Z

nibabel/gifti/parse_gifti_fast.py

-    out.img.filename = fname
-    return out.img
+    def initialize(self, *args, **kwargs):
+        self.__init__(*args, **kwargs)


Outputter.__init__ doesn't quite do the same thing as Outputter.initialize, since it also calls super(GiftiImageParser, self).__init__, which could change the buffer size and all that. I would just do the original contents, but short, like:

def initialize(self): """ Initialize outputter """ self.fsm_state = [] self.nvpair = self.da = self.coordsys = self.lata = self.label = None self.meta_global = self.meta_da = self.write_to = None self._char_blocks = None self.count_da = True

Setting encoding and buffer_size is new. I figured since initialize is deprecated, nobody will call it with those args, nor would those args be non-defaults. I'd prefer to keep the # of lines for deprecated functions minimal.

As a side note, I could remove the arguments from initialize and __init__ to match the original definitions.

LMK what you think.

I would keep the original prototypes and docstring on initialize. But point taken on only defaults being used in any case, so revert the prototypes and I'm okay here.

bcipolli · 2015-11-05T12:29:30Z

Done.

effigies · 2015-11-05T12:35:21Z

nibabel/gifti/parse_gifti_fast.py

-    else:
-        datasource = fptr
+class Outputter(GiftiImageParser):
+    @np.deprecate_with_doc("Use GiftiImageParser instead.")


Should this deprecation be on the class instead of __init__?

I probably don't understand what deprecating the class would do; could you help make that clear to me?

In previous cases I remember deprecating __init__ so that the deprecation is lazy--it only triggers if someone instantiates the class, and not when the file is simply imported. That's the only goal here.

I thought the @np.deprecate functions modify documentation. Do they also raise warnings? If so, this makes sense.

yep, they raise warnings--I add a deprecation warning test case each time we add this decorator, to make sure.

effigies · 2015-11-05T13:08:42Z

Okay, LGTM then.

Anybody else have comments?

bcipolli · 2015-11-21T16:50:39Z

Any chance we can merge this one?

effigies · 2015-11-22T22:36:02Z

I think people have had a chance to voice objections. In it goes.

RF: Use XmlSerializable, XmlParser for Gifti load/save

matthew-brett · 2015-11-22T22:37:16Z

Yes, sorry I haven't had a chance to review. Next week is better for me, by Tuesday afternoon I will have more time.

matthew-brett · 2015-11-29T00:56:52Z

nibabel/gifti/gifti.py

-                                file_map=file_map)
+        super(GiftiImage, self).__init__(header=header, extra=extra,
+                                         file_map=file_map)
+        # placed here temporarily for git diff purposes


Rewrite comment now you've decided in this implementation?

effigies · 2015-11-29T02:33:19Z

@matthew-brett Should I revert this merge?

matthew-brett · 2015-11-29T02:51:17Z

Oh - no - sorry - you did good to merge, thanks for doing it. I was only adding post-merge suggestions. Happy to do that as a PR if it's easier.

RF: Use XmlSerializable, XmlParser for Gifti load/save

Ben Cipollini added 8 commits October 24, 2015 10:52

xmlutils.py => xmlbasedimages.py

7853fa9

Base GiftiImage on XmlBasedImage

20d8a38

Use gifti's to_file_map as XmlBasedImage's default implementation.

957f799

Create a new XmlImageParser abstract class, which must be implemented…

abcf925

… and attached to each XmlBasedImage subclass.

Implement GiftiImageParser, stamp on GiftiImage

f13bbec

Fix: GiftiImage should use get_/set_filename

3cdbe63

Properly deprecate Outputter, parse_gifti_file

51adb8a

BF: re-implement buffer size; makes parsing faster!

853508e

bcipolli reviewed Oct 26, 2015
View reviewed changes

Ben Cipollini added 4 commits October 26, 2015 14:28

TST: Add parse_gifti_fast deprecation tests (and fix old deprecation …

ceb6f72

…tests)

Use verbose=1 instead of global DEBUG_PRINT flag

6f8d65b

STY: linting parse_gifti_fast

2ac9a9f

STY, DOC: linting, documentation.

f5fa1a4

Allow buffer_size=None, to avoid setting explicitly.

c3dfb07

FIX: xmlbasedimages

10b6381

effigies reviewed Nov 4, 2015
View reviewed changes

Ben Cipollini added 2 commits November 4, 2015 14:44

Remove XmlBasedImage, xmlbasedimages.py=>xmlutils.py

a17f26f

Deprecate (not delete) Outputter.initialize.

7410d89

effigies reviewed Nov 4, 2015
View reviewed changes

Match previous interface for Outputter, add back minimal docstring.

5c850da

bcipolli changed the title ~~WIP: XmlBasedImage, XmlImageParser~~ MRG: XmlBasedImage, XmlImageParser Nov 5, 2015

effigies reviewed Nov 5, 2015
View reviewed changes

bcipolli changed the title ~~MRG: XmlBasedImage, XmlImageParser~~ RF: Use XmlSerializable, XmlParser for Gifti load/save Nov 20, 2015

effigies added a commit that referenced this pull request Nov 22, 2015

Merge pull request #365 from bcipolli/xmlbasedimages

fde94f1

RF: Use XmlSerializable, XmlParser for Gifti load/save

effigies merged commit fde94f1 into nipy:master Nov 22, 2015

matthew-brett reviewed Nov 29, 2015
View reviewed changes

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

Merge pull request nipy#365 from bcipolli/xmlbasedimages

ef20fae

RF: Use XmlSerializable, XmlParser for Gifti load/save

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RF: Use XmlSerializable, XmlParser for Gifti load/save #365

RF: Use XmlSerializable, XmlParser for Gifti load/save #365

bcipolli commented Oct 23, 2015

bcipolli Oct 26, 2015

bcipolli commented Oct 26, 2015

bcipolli commented Oct 27, 2015

effigies commented Oct 27, 2015

effigies Nov 4, 2015

effigies commented Nov 4, 2015

bcipolli commented Nov 4, 2015

effigies commented Nov 4, 2015

bcipolli commented Nov 4, 2015

effigies Nov 4, 2015

bcipolli Nov 4, 2015

effigies Nov 4, 2015

bcipolli commented Nov 5, 2015

effigies Nov 5, 2015

bcipolli Nov 5, 2015

effigies Nov 5, 2015

bcipolli Nov 5, 2015

effigies commented Nov 5, 2015

bcipolli commented Nov 21, 2015

effigies commented Nov 22, 2015

matthew-brett commented Nov 22, 2015

matthew-brett Nov 29, 2015

effigies commented Nov 29, 2015

matthew-brett commented Nov 29, 2015

RF: Use XmlSerializable, XmlParser for Gifti load/save #365

RF: Use XmlSerializable, XmlParser for Gifti load/save #365

Conversation

bcipolli commented Oct 23, 2015

Choose a reason for hiding this comment

bcipolli commented Oct 26, 2015

bcipolli commented Oct 27, 2015

effigies commented Oct 27, 2015

Choose a reason for hiding this comment

effigies commented Nov 4, 2015

bcipolli commented Nov 4, 2015

effigies commented Nov 4, 2015

bcipolli commented Nov 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcipolli commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Nov 5, 2015

bcipolli commented Nov 21, 2015

effigies commented Nov 22, 2015

matthew-brett commented Nov 22, 2015

Choose a reason for hiding this comment

effigies commented Nov 29, 2015

matthew-brett commented Nov 29, 2015