Skip to content
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

minor bugfixes on aa_export_toBIDS #196

Merged
merged 2 commits into from May 29, 2019

Conversation

Projects
None yet
2 participants
@jones-michael-s
Copy link
Collaborator

commented Apr 1, 2019

Took export_to_BIDS out for a test drive. Here's a few bugfixes...

@tiborauer

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

It seems quite all right, but I am not so sure about

copyfile(src,dest); gzip(dest); delete(dest); if exist('src_dummy','file'), delete(src_dummy); clear src_dummy; end

because it deletes src_dummy from which the pipeline but leaves src, the temporary file.

@jones-michael-s

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 1, 2019

I could be wrong but don't we want to keep src? In my test pipeline src is the .nii that gets generated at the end of aamod_covert_epi.

I discovered this problem by running export_toBIDS twice. On the second run, the copyfile crashes because src was deleted from aamod_convert_epi in the previous pass.

@tiborauer

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

It depends on the use case:

  1. If there is no dummy scan, then src is indeed the converted epi. In that case, src_dummy will not be created and nothing will be deleted.
  2. If there is any dummy scan, then src and src_dummy point to the temporary file (dummy + epi) and the dummy file, respectively,
@jones-michael-s

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2019

Are you able to run export_toBIDS twice on your data? This is what alerted us to the problem.

@tiborauer

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Yes, I can run aa_export_toBIDS twice on e.g. both the aa demo and the bids114 datasets. However, both requires some tweaking:

  • aa demo has a short subjectname, which causes
    if ~isequal(suboutname(1:4),'sub-')
    crash, so it has to be omited.
  • bids114 demo has not got a full header which causes
    'PhaseEncodingDirection',[...
    sliceaxes{cell_index(sliceaxes(:,1),deblank(hdr.InPlanePhaseEncodingDirection)),2}{aas_get_numaris4_numval(hdr.CSAImageHeaderInfo,'PhaseEncodingDirectionPositive')+1}...
    ],...
    crash, so they have to be omited.

However, both issues are irrrelevant regarding the current PR. Still, they should be fixed, of course.

@jones-michael-s

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2019

Yes, I can run aa_export_toBIDS twice on e.g. both the aa demo and the bids114 datasets.

Interesting. I'll look into what is peculiar about my data (which crashes)...

@jones-michael-s

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

Okay, finally got some time to look into this. I think the line in question should actually read:

copyfile(src,dest); gzip(dest); delete(dest); if (exist('src_dummy','var') && ~isempty(src_dummy)), delete(src); clear src_dummy; end

The hiccup is src_dummy gets created by checking if any of the modules have a "dummyscan" output stream (line 225) -- this will be true for any pipeline that contains aamod_convert_epis even if you don't actual create any dummyscans. So we need an additional check if scr_dummy is empty. That fixed the crash on my machine.

@tiborauer

This comment has been minimized.

Copy link
Member

commented May 27, 2019

This looks good to me. Do you want to update the PR?

@jones-michael-s

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2019

This looks good to me. Do you want to update the PR?

Nah -- it was my screw up.

I'll push a correction...

@tiborauer tiborauer merged commit e90bde7 into automaticanalysis:master May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.