-
Notifications
You must be signed in to change notification settings - Fork 265
ENH: Add dtype argument to Cifti2Image #1111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tests forthcoming. |
Codecov Report
@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
+ Coverage 92.26% 92.28% +0.01%
==========================================
Files 100 100
Lines 12269 12285 +16
Branches 2399 2402 +3
==========================================
+ Hits 11320 11337 +17
Misses 624 624
+ Partials 325 324 -1
Continue to review full report at Codecov.
|
c2dbd89
to
70579bc
Compare
70579bc
to
7933fae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple notes for a reviewer.
@@ -103,6 +123,10 @@ def _underscore(string): | |||
return re.sub(r'([a-z0-9])([A-Z])', r'\1_\2', string).lower() | |||
|
|||
|
|||
class LimitedNifti2Header(Nifti2Header): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly there's a better name. _Cifti2AsNiftiHeader
exists to do some validation during parsing, but isn't the type of nifti_header
.
nibabel/nibabel/cifti2/parse_cifti2.py
Lines 78 to 119 in 7933fae
class _Cifti2AsNiftiHeader(Nifti2Header): | |
""" Class for Cifti2 header extension """ | |
@classmethod | |
def _valid_intent_code(klass, intent_code): | |
""" Return True if `intent_code` matches our class `klass` | |
""" | |
return intent_code >= 3000 and intent_code < 3100 | |
@classmethod | |
def may_contain_header(klass, binaryblock): | |
if not super(_Cifti2AsNiftiHeader, klass).may_contain_header(binaryblock): | |
return False | |
hdr = klass(binaryblock=binaryblock[:klass.sizeof_hdr]) | |
return klass._valid_intent_code(hdr.get_intent('code')[0]) | |
@staticmethod | |
def _chk_qfac(hdr, fix=False): | |
# Allow qfac of 0 without complaint for CIFTI-2 | |
rep = Report(HeaderDataError) | |
if hdr['pixdim'][0] in (-1, 0, 1): | |
return hdr, rep | |
rep.problem_level = 20 | |
rep.problem_msg = 'pixdim[0] (qfac) should be 1 (default) or 0 or -1' | |
if fix: | |
hdr['pixdim'][0] = 1 | |
rep.fix_msg = 'setting qfac to 1' | |
return hdr, rep | |
@staticmethod | |
def _chk_pixdims(hdr, fix=False): | |
rep = Report(HeaderDataError) | |
pixdims = hdr['pixdim'] | |
spat_dims = pixdims[1:4] | |
if not np.any(spat_dims < 0): | |
return hdr, rep | |
rep.problem_level = 35 | |
rep.problem_msg = 'pixdim[1,2,3] should be zero or positive' | |
if fix: | |
hdr['pixdim'][1:4] = np.abs(spat_dims) | |
rep.fix_msg = 'setting to abs of pixdim values' | |
return hdr, rep |
@@ -674,7 +716,12 @@ def header_maker(self): | |||
return self.image_maker.header_class() | |||
|
|||
|
|||
class TestAnalyzeAPI(ImageHeaderAPI): | |||
class TestSpatialImageAPI(ImageHeaderAPI): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This subclass/superclass swap occurs because not all spatial images have dtype overriding.
If anybody wants to review, please let me know by end of day (5pm UTC-4). Otherwise I'll merge and aim to release 4.0.0rc0 on Monday. |
To bring into line with other Analyze-like formats.
Given that the subject that brought us here was inappropriate use of image dtypes, I am also constraining the dtypes to those listed in the standard:
I also dropped float128, which is not specifically restricted, but nor is it specifically permitted. Seems unlikely to be a good idea.