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

Replace deprecated "_phase" suffix in reproin heuristic with "part-phase" entity #770

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ target output directory during conversion.
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
hat BIDSsupports the part entity
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
Expand Down
56 changes: 47 additions & 9 deletions heudiconv/heuristics/reproin.py
Original file line number Diff line number Diff line change
Expand Up @@ -402,15 +402,43 @@
run_label: Optional[str] = None # run-
dcm_image_iod_spec: Optional[str] = None
skip_derived = False
for curr_seqinfo in seqinfo:
for i_acq, curr_seqinfo in enumerate(seqinfo):
# XXX: skip derived sequences, we don't store them to avoid polluting
# the directory, unless it is the motion corrected ones
# (will get _rec-moco suffix)
if skip_derived and curr_seqinfo.is_derived and not curr_seqinfo.is_motion_corrected:
if (
skip_derived
and curr_seqinfo.is_derived
and not curr_seqinfo.is_motion_corrected
):
skipped.append(curr_seqinfo.series_id)
lgr.debug("Ignoring derived data %s", curr_seqinfo.series_id)
continue

if i_acq == 0:
prev_dcm_image_iod_spec = None
else:
prev_seqinfo = seqinfo[i_acq - 1]
for f in "series_description", "protocol_name":
prev_seqinfo = prev_seqinfo._replace(
**{f: getattr(prev_seqinfo, f).format(**prev_seqinfo._asdict())}
)

prev_dcm_image_iod_spec = prev_seqinfo.image_type[2]

if i_acq == (len(seqinfo) - 1):
next_series_description = None
next_dcm_image_iod_spec = None
else:
next_seqinfo = seqinfo[i_acq + 1]
for f in "series_description", "protocol_name":
next_seqinfo = next_seqinfo._replace(
**{f: getattr(next_seqinfo, f).format(**next_seqinfo._asdict())}
)

next_series_description = next_seqinfo.series_description
next_dcm_image_iod_spec = next_seqinfo.image_type[2]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this duplicates that _replace hack 2 more times, causes me duplication-allergy...

Since we are to do that anyways, do it once then -- just in a loop before this loop apply the replacements to all first.

Then avoid that i_acq - do following 'zip'ing, as chatgpt suggested

my_list = [1, 2, 3, 4, 5]

for previous, current, next in zip([None] + my_list[:-1], my_list, my_list[1:] + [None]):
    print(f"Previous: {previous}, Current: {current}, Next: {next}")

which would avoid all the if's and else's etc and make code easier to read IMHO.

# possibly apply present formatting in the series_description or protocol name
for f in "series_description", "protocol_name":
curr_seqinfo = curr_seqinfo._replace(
Expand All @@ -423,7 +451,6 @@

# figure out type of image from curr_seqinfo.image_info -- just for checking ATM
# since we primarily rely on encoded in the protocol name information
prev_dcm_image_iod_spec = dcm_image_iod_spec
if len(curr_seqinfo.image_type) > 2:
# https://dicom.innolitics.com/ciods/cr-image/general-image/00080008
# 0 - ORIGINAL/DERIVED
Expand Down Expand Up @@ -500,10 +527,6 @@
if datatype == "func":
if "_pace_" in series_spec:
datatype_suffix = "pace" # or should it be part of seq-
elif "P" in curr_seqinfo.image_type:
datatype_suffix = "phase"
elif "M" in curr_seqinfo.image_type:
datatype_suffix = "bold"
else:
# assume bold by default
datatype_suffix = "bold"
Expand All @@ -523,6 +546,19 @@
# label for dwi as well
datatype_suffix = "dwi"

# Add "part" entity as needed
if datatype != "fmap" and not curr_seqinfo.series_description.endswith(
"_SBRef"
):
if "P" in curr_seqinfo.image_type:
series_info["part"] = "phase"

Check warning on line 554 in heudiconv/heuristics/reproin.py

View check run for this annotation

Codecov / codecov/patch

heudiconv/heuristics/reproin.py#L554

Added line #L554 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov says that this line is not covered, not yet sure how wide implication is ;-)

elif "M" in curr_seqinfo.image_type:
# if next one is phase from same scan, we should set part to mag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it guaranteed to be the "next", could it be "prior"?

if (
next_series_description == curr_seqinfo.series_description
) and (next_dcm_image_iod_spec == "P"):
series_info["part"] = "mag"

Check warning on line 560 in heudiconv/heuristics/reproin.py

View check run for this annotation

Codecov / codecov/patch

heudiconv/heuristics/reproin.py#L560

Added line #L560 was not covered by tests

#
# Even if datatype_suffix was provided, for some data we might need to override,
# since they are complementary files produced along-side with original
Expand Down Expand Up @@ -617,8 +653,10 @@
from_series_info("dir"),
series_info.get("bids"),
run_label,
from_series_info("part"),
datatype_suffix,
]

# filter those which are None, and join with _
suffix = "_".join(filter(bool, filename_suffix_parts)) # type: ignore[arg-type]

Expand All @@ -643,8 +681,8 @@
"_Scout" in curr_seqinfo.series_description
or (
datatype == "anat"
and datatype_suffix
and datatype_suffix.startswith("scout")
and filename_suffix_parts[-1]
and filename_suffix_parts[-1].startswith("scout")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this have to do with phase suffix? nothing in short commit messages hints on possible relation to this change... why is it?

)
or (
curr_seqinfo.series_description.lower()
Expand Down