BF: split prefix into path (if any) and provide only basename as prefix for dcm2niix invocation #485
Conversation
This file contains 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
…ix for dcm2niix invocation dcm2niix started to segfault whenever -f is too long. rordenlab/dcm2niix#465 And it feels that we abused -f wherever -o should have been used. I have not looked though through history of changes if such use of -f was warranted, but as I see that output_dir is not explicitely defined, decided to just use that one instead. this seems to resolve the dcm2niix segfault during heudiconv/tests/test_heuristics.py::test_scans_keys_reproin when testing on my laptop
Original motivation is that somehow prior commits trying to split apart prefix from the full path we are already providing broke some tests, e.g. resulted in extra commits on rewrites. Since they could be one the same file system and otherwise not used after being transfered to the target destination, it would be much cheaper to just move them instead of copying. It is also more likely to have sufficient space in destination than in the /tmp, which already brought questions: this commit would Closes nipy#481 as a result as well.
I think this is Closes nipy#462
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
==========================================
+ Coverage 75.96% 76.01% +0.04%
==========================================
Files 38 38
Lines 2996 3010 +14
==========================================
+ Hits 2276 2288 +12
- Misses 720 722 +2
Continue to review full report at Codecov.
|
|
Dear @mgxd @tsalo @satra and @nipy/team-heudiconv -- your speedy feedback on this change would be appreciated. We need to workaround a regression (IMHO) in dcm2niix which could lead to segfaults with current version. See description of PR and individual commits for more information. If no objections are voiced within a day, I would aim to release tomorrow (sorry for a short notice etc, but want to get this one out) and will bear the responsibility for fixing what I broke (if anything) |
yarikoptic
commented
Dec 22, 2020
|
FWIW, wherever dependencies allowed, builds went find on neurodebians |
yarikoptic
added a commit
that referenced
this pull request
Dec 23, 2020
Various improvements and compatibility/support (dcm2niix, datalad, duecredit) changes. Major change is placement of output files to the target output directory during conversion. - #454 zenodo referencing in README.rst and support for ducredit for heudiconv and reproin heuristic - #445 more tutorial references in README.md - [#485][] placed files during conversion right away into the target directory (with a `_heudiconv???` suffix, renamed into ultimate target name later on), which avoids hitting file size limits of /tmp ([#481][]) and helped to avoid a regression in dcm2nixx 1.0.20201102 - #477 replaced `rec-<magnitude|phase>` with `part-<mag|phase>` now that BIDS supports the part entity - #473 made default for CogAtlasID to be a TODO URL - #459 made AcquisitionTime used for acq_time scans file field - #451 retained sub-second resolution in scans files - #442 refactored code so there is now heudiconv.main.workflow for more convenient use as a Python module - minimal version of nipype set to 1.2.3 to guarantee correct handling of DWI files ([#480][]) - `heudiconvDCM*` temporary directories are removed now ([#462][]) - compatibility with DataLad 0.13 ([#464][]) - #443 pathlib as a dependency (we are Python3 only now) * tag 'v0.9.0': Add a helper rule to upload to pypi update changelog reference as part of prep release [DATALAD RUNCMD] prepare the release CHANGELOG entry for 0.9.0
yarikoptic
added a commit
that referenced
this pull request
Dec 23, 2020
Various improvements and compatibility/support (dcm2niix, datalad, duecredit) changes. Major change is placement of output files to the target output directory during conversion. - #454 zenodo referencing in README.rst and support for ducredit for heudiconv and reproin heuristic - #445 more tutorial references in README.md - [#485][] placed files during conversion right away into the target directory (with a `_heudiconv???` suffix, renamed into ultimate target name later on), which avoids hitting file size limits of /tmp ([#481][]) and helped to avoid a regression in dcm2nixx 1.0.20201102 - #477 replaced `rec-<magnitude|phase>` with `part-<mag|phase>` now that BIDS supports the part entity - #473 made default for CogAtlasID to be a TODO URL - #459 made AcquisitionTime used for acq_time scans file field - #451 retained sub-second resolution in scans files - #442 refactored code so there is now heudiconv.main.workflow for more convenient use as a Python module - minimal version of nipype set to 1.2.3 to guarantee correct handling of DWI files ([#480][]) - `heudiconvDCM*` temporary directories are removed now ([#462][]) - compatibility with DataLad 0.13 ([#464][]) - #443 pathlib as a dependency (we are Python3 only now) * tag 'v0.9.0': Do no bother ensuring that version changed - should be no changes
2 tasks
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.
dcm2niix started to segfault whenever -f is too long rordenlab/dcm2niix#465 .
And it feels that we abused -f wherever -o should have been used.
I have not looked though through history of changes if such use of -f
was warranted, but as I see that output_dir is not explicitly defined,
decided to just use that one instead.
Initial commit resolved the dcm2niix segfault during heudiconv/tests/test_heuristics.py::test_scans_keys_reproin
when testing on my laptop, BUT resulted in test failures since now placing the files in the output directory copying to destination was not working since it was destination. I added conditional in
safe_copyfile(0f6abbf) but that was not sufficient and also made me wonder -- if placing already at the destination (or near) why don't we move instead of copying? That would also resolve #481 and make heudiconv more robust IMHO in cases where/tmphas smaller capacity than the destination.The only negative side-effect might be that the target directory, in case of abnormal heudiconv operation, would have those
_heudiXXXfiles. I could present it as a feature which would ease troubleshooting etc ;)So I followed up with 8b4b7a0 where I have also added a random suffix to the target suffix given to nipype's dcm2niix call. This is the most radical change in this PR which might introduce some bugs if we are not moving/renaming files properly. But looking at the code we seems to be doing all needed, and then
as in that explicit test we do not bother renaming etc, whenever all the other files in there seems to look good. So I have good confindence that we should be ok.
then while at it I followed up with 7ad241c which should address #462 .
TODO
-fwe need it fast though segfaults upon some "too long" path within -f rordenlab/dcm2niix#465 so I will be merging/releasing as soon as I am satisfied)edit: additional benefit