-
Notifications
You must be signed in to change notification settings - Fork 260
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
Problem loading large CIFTI2 files in lastest updates #520
Comments
I know it's post-merge cause when I run it from the corresponding merge commit everything works. I will try and run from that commit later. Is there a way to establish tests for these cases? |
How are you detecting the memory usage at the moment? Maybe we could use something similar for a test? |
Because I get the exception I think that the key problem for the test is to have a large enough file on the test battery. |
OK, it does work from 6f58e02 too. I tried tracing back the error but it seems to be deep into the memorymap and lazy loading data mechanisms |
Just to confirm - you mean there is no memory error from 6f58e02 ? I'm guessing you are on Python 2.7? Would you mind checking whether you get the memory error on Python 3, for this commit? @effigies - I wonder whether we should just revert the |
There's no memory error from commit 6f58e02 I'm working on python 3.5 not on 2.7. |
Sure. Let's revert and just fix the import from externals.
Feel free to do it, or I can when I get to a computer.
Should we generally switch back to six.BytesIO? The main impulse I was
indulging was a desire for consistency.
|
@demianw - that's really odd, that you get this change in behavior on Python 3.5 - Chris' recent refactoring should only have an effect for Python 2. What do you get when you checkout the master branch again, and you apply this patch:
? |
@matthew-brett Mmmmmm. OK, I cannot reproduce the error now. The computer where I'm running everything is under much less memory stress (external conditions). Yet I dealt with this issue thorough the day of yesterday and today's morning (on a script that had worked flawlessly for weeks). Thanks for such quick replies and I'll work on developing a test where we can reproduce this on other computers before crying wolf again. |
@demianw If you could test pre and post refactor on each 3.5 and 2.7, that
would be great. Thanks for the report.
|
Well, the best thing will be to try and add a memory stress test to the test battery and let travis do the work ;) |
@demianw - are you sure you weren't using Python 2 when you got the error? |
@matthew-brett @effigies I'm absolutely sure I'm on python 3. Also the error happens again but only when the machine is under memory stress. Here you have the traceback which should help
|
Can you also confirm you don't see this error with 6f58e02 ? |
@matthew-brett That error does not look likely to be caused by any uses of |
@effigies is right it actually happens intermittently in all commits. Is there a way I can modify nibabel/nibabel/cifti2/parse_cifti2.py Line 123 in ca977ab
Nifti2Image and performs lazy loading of the data?
Maybe it's line 150 that needs to be changed? nibabel/nibabel/cifti2/parse_cifti2.py Line 150 in ca977ab
|
Line 150 looks like it can be removed. @demianw Do you want to try removing it and seeing if you're still able to load your files at all? And then under low-memory conditions? |
Thanks! Done that, things work for small pieces. But I still get a problem which is not there when loading the files with
|
I'm going to take a guess, and say the issue is that we can't partially parse the XML portion of the CIFTI file. By loading it as NIfTI-2, you can leave the data as a block on disk and access it when needed. In order to actually construct the CIFTI structure, you have to fully load it. So I think what would be needed to lazily load:
In the absence of the former, one approach could be to maintain an offset table, and when contents are returned, search forward from the last offset for the current contents. |
This doesn't sound that logical cause I can load the image as a NIfTI-2 and get the XML from the NIfTI-2 header without a problem and parse it. (CIFTI-2 is just a particular case of NIFTI-2 with the XML in the header and the 4 first dimensions set to 1) |
Ah, I misunderstood. I thought the XML was in the data block, like Gifti.
As it's not, it may still be the case we need to switch to lazy loading of
sub-blocks, but I can't speak with any confidence here.
…On Mar 24, 2017 11:37 AM, "Demian Wassermann" ***@***.***> wrote:
This doesn't sound that logical cause I can load the image as a NIfTI-2
and get the XML from the NIfTI-2 header without a problem and parse it.
(CIFTI-2 is just a particular case of NIFTI-2 with the XML in the header
and the 4 first dimensions set to 1)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#520 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFF8hCLXBFLM-FZC8Z_kJowYPpY4--rks5ro-NLgaJpZM4Mn96p>
.
|
It's weird because the CIFTI-2 class is just a wrapper around the NIFTI-2 one discarding the first for dimensions of the data. Loading the CIFTI-2 images as NIFTI-2 (through I think the bug should be in that section but the lazy loading system is a bit too hard for me to get in the amount of time I have available. |
So, it looks like you should be able to reproduce the error with: from nibabel import Nifti2Image, reshape_dataobj
img = Nifti2Image.load(fname)
assert img.shape[:4] == (1, 1, 1, 1)
dataobj = reshape_dataobj(img.dataobj, img.shape[4:]) So it's not a And my reading is that because there is no (I hope I'm not as wrong as last time...) |
Yup! I think that you've hit the right nail this time! |
Can you try 4c4405b? (May need to fetch from my repo.) If you don't throw out the original |
Great! I'm having trouble locating the branch / repo for that commit in the github interface |
git remote add effigies https://github.com/effigies/nibabel.git
git fetch effigies
git checkout 4c4405b If you want the branch instead of the commit, it's |
Doesn't work
|
Right. Sorry about that. Re-fetch and try: 1eb6fbe |
Need to jump to something else will try tomorrow |
OK, this works now. We just need to add this patch
|
Hi!
it seems that one of the post-"Merge pull request #249" updates to the codebase is forcing the
load
function of cifti2 to bring into memory the whole loaded matrix. For large files (I'm working on files of ~10Gb) this forces a "cannot allocate memory" exception.I don't know how is it possible to add such large files to the test-base such that we can have a control for this. In any case it will be great if the error could be fixed.
The text was updated successfully, but these errors were encountered: