Binary header class #50
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Preparing for your actual binary header class, and it's own test_binary testing code. Move analyze-specific tests into test_analyze Adapt testing of other headers to use self.log_chk (renamed from _log_chk)
Split the Analyze header into a WrapStruct class, that wraps a structured array, and the features specific to the Analyze header, including general get_data_dtype etc.
|
That looks like it is also useful to add support for NIfTI2, right? |
|
Well - I'm hoping that Nifti2 could be as simple as subclassing Nift1Image and Nifti1PairImage and subclassing nifti1header changing the dtype definition, and probably a few of the checks (the magic has changed?) and the empty_header (for the magic again?). |
Trying to make the wrapstruct more independent of the header idea, in the docs
The name more clearly expresses the meaning; it also matches the public property ``structarr``
The name more clearly expresses what the method is for. The method is common enough that it seems reasonable to promote it to be part of the public API.
Separating WrapStructs from headers at a conceptual level. Subclasses such as Analyze raise HeaderDataErrors if they want them trapped by their own error machinery, but wrapstructs raise their own errors internally. In fact, the only time this happens at the moment is checking the size of the binary block passed in.
I was using an int32, that numpy doctests show as e.g ``array(3)`` on 32-bit systems, but ``array(3, dtype=int32)`` on 64 bit systems. Changing to int16 means the dtype always appears in the output.
It makes more sense for this method not to require an instance.
Guessed endian needs to be written for each substantial subclass, so it should be part of the API. It makes more sense as a classmethod because we should not need an instance to test other data for endianness.
We use cls._dtype in various places; it should be part of the public API.
The field_recoders is a bit specific to headers; move to the header classes, leave wrapstruct with generic stub interface to get_value_label
It was previously raising AttributeErrors trying to find the parts to compare.
|
Any more comments on this one? |
|
thanks for the 32/64 bit fix.. looks good |
|
Thanks for the reviews guys - merging. |
matthew-brett
added a commit
that referenced
this pull request
Sep 23, 2011
Abstract handling of binary structures into WrapStruct class
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Split Analyze header object into WrapStruct - a general structured array carrying object, and the AnalyzeHeader, which extends WrapStruct by adding header-like and Analyze specific attributes.
This should make it easier for @cindeem to use in ecat.py - if you like...