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

try a simple fix for wrongly ordered files in tar file #535

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

bpinsard
Copy link
Contributor

@bpinsard bpinsard commented Nov 5, 2021

solves #519

@codecov
Copy link

codecov bot commented Nov 5, 2021

Codecov Report

Merging #535 (fcee77a) into master (80a6538) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #535   +/-   ##
=======================================
  Coverage   77.64%   77.64%           
=======================================
  Files          41       41           
  Lines        3167     3167           
=======================================
  Hits         2459     2459           
  Misses        708      708           
Impacted Files Coverage Δ
heudiconv/parser.py 92.70% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80a6538...fcee77a. Read the comment docs.

@bpinsard bpinsard linked an issue Nov 5, 2021 that may be closed by this pull request
2 tasks
@yarikoptic
Copy link
Member

But isn't the underlying issue that we (or is it a heuristic?) have some order effect?

@yarikoptic
Copy link
Member

File names could be 1, 2, ..., 10 and string sorting would screw it up

@bpinsard
Copy link
Contributor Author

bpinsard commented Nov 5, 2021

You're right, our files are alphanumerically sorted by acquisition/content-creation order because it uses Siemens UID, and that might not be the general case.
However, I think it is sorted as well when not pulled from a tar filed (eg obtained from a directory).

files += sorted(find_files(
'.*', topdir=f, exclude_vcs=True, exclude="/\.datalad/"))

In most cases the order of the files won't matter to the indexing nor to dcm2niix.

But for that particular case described in #519 : complex data (at least 2 dicoms with M and P ImageType) in a single series will give a varying ImageType to the seqinfo depending on the sorting, as the parse will choose the first one as the example dicom.

@yarikoptic
Copy link
Member

your argument that we sort in non-tar handling convinces me. Let's proceed with this, and see if any fallout happens ;) Thank you @bpinsard !

@yarikoptic yarikoptic merged commit 3f9a504 into nipy:master Feb 2, 2022
@bpinsard bpinsard deleted the fix/tar_unordered_files branch February 4, 2022 21:32
@yarikoptic yarikoptic added the patch Increment the patch version when merged label Apr 20, 2022
@github-actions
Copy link

🚀 PR was released in v0.11.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inconsistent Seqinfos when extracted from tarfile with mag+phase SBRef
2 participants