MRG: working on top of Eric's PARREC changes #261
Conversation
Help strings got a bit wonky with the PEP8 changes.
``array_to_file`` in fact accepts array-like.
Let ``array_to_file`` cast data to array if necessary. Don't need to use ``write0`` flag to seek_tell because output file type can only be uncompressed or .gz; let ``array_to_file`` do seek.
Add nibabel-data directory and routines to find it / use it.
Load PAR images and compare to NIfTI image when we have them. Raise SkipTest for fieldmap image, which doesn't match the NIfTI.
Convert all the balls images we can and test against previous nifti conversions.
Stick the verbose flag on the function itself for neatness.
The code was setting the header extensions into the image, but this doesn't have any effect on writing the header, and we aren't using the image for anything.
Refactor mriutils dwell time calculation to raise errors instead of returning None. Do simple tests. Remove calculate_dwell_time from top level (but import mriutils module).
Don't default to 3T - it would be very easy for someone using the script not to notice that default and then get incorrect values.
Test overwrite, bvs, dwell-time options to parrec2nii script.
I thought about this for a while, but I don't like it, because it changes the behavior of nibabel globally. Previously if you asked to open ``my_image.NII`` it would barf, now it might find an image when it previously it did not. This is probably what the user intended, but it's still a guess. It's not hard to specify the upper case file extension on systems that need it. The next commit adds an explicit function to ignore extension case for parrec2nii.
Restore ignoring case in file extensions by using an explicit function.
|
Looking at the commits this sounds reasonable to me. What should be the plan here? Options:
WDYT? |
Makes testing easier. I think the code is easier to read.
matthew-brett
changed the title
``get_voxel_size` only being used in ``_calc_zooms`` method; fold code into that method, deprecate ``get_voxel_size``.
I think the previous code was incorrect comparing 2D arrays, and I found it hard to follow. Refactor to do simple diff to find any differing rows in the array; return scalar if selected array is 1D.
Testing warnings can be really annoying, because once the warning has been raises, by default, it cannot be raised again, because it is already in the warnings registry. Here is a context manager to reset the warnings registry inside the context manager. The general idea was inspired by the conversation here: https://stackoverflow.com/questions/2390766/how-do-i-disable-and-then-re-enable-a-warning
Rename the ``sorted_slice_indices`` property to a ``get_sorted_slice_indices`` method, and test.
We were caching slice_orientation, but the other ``get_`` methods do not cache and get their values from the current values of ``self.general_info`` and ``self.image_defs``. Do the same for ``get_slice_orientation`` so that the values are always up to date with the image attributes.
The input parameters are small, and it is easy to get confused when using the mutable input parameters elsewhere, so copy the input dict, array so each header has their own copy.
matthew-brett
force-pushed
the
working-on-erics-pr
branch
from
1a550fb
to
c3e7d97
Compare
|
@matthew-brett will your context manager conflict with my PR here: ? |
|
|
||
| def test_get_voxel_size_deprecated(): | ||
| hdr = PARRECHeader(HDR_INFO, HDR_DEFS) | ||
| with catch_warn_reset(modules=[parrec]): |
There was a problem hiding this comment.
I usually do with warnings.catch_warnings(record=True) as w: and then assert something about w. It should be fewer lines than having to call simplefilter('error') every time, and presumably more standard
|
Pull 254 does give merge conflicts, yes - I'm happy to clear those up when merging though. I don't have a strong feeling about record=True, I have used both, if you like, I can change it. |
|
Up to you, it's your package :) I am +1 for consistency, so if Latest round of commits look reasonable to me, so I'm happy with merge + work on |
``get_voxel_size`` needs to use new API for get_unique_image_props.
Check wlist instead of raising an error.
|
Looks like this just needs a rebase, then it should be good to go, right? I was hoping to take a crack at working on the data tomorrow at some point. It would be ideal if I could work off of |
|
Merged rebased version as commit 7428ec4 - thanks for all the work Eric - PAR / REC is vastly improved. |
|
Awesome! I'll see if I can make some progress on changes necessary for our files tomorrow. |
|
Eric - any progress? I'd like to release next week. |
|
Closing this PR in favor of rebased version merged with 7428ec4 |
|
Oh, I thought you had already merged these changes. No progress thus far from my end -- do you need to wait for my next round |
|
Yes, sorry to confuse, I have merged these changes using a rebased version. If you think you'll have time to get to this early next week please do, we might be able to shake out any extra bugs with some new images to test against. |
|
I need to confer with our local expert @mrjeffs to confirm, but most (if not all) of the bugs we had before have been fixed through your changes, which is great. I doubt he will have time today or tomorrow to look at the files, so you don't need to delay release on our behalf. However, #263 would be good to include in the release, since it fixes some broken tests. It would be awesome to spend a bit of time getting #251 (and the related matthew-brett#5) if you think we can get it stable enough... |
New version of #253 with more changes on
top.
Please see original PR for comments.
This PR rebased on master.