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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
55f21bb
Work on part.
tsalo Jun 14, 2024
b00e41f
Get it working.
tsalo Jun 21, 2024
168e078
Remove perfusion datatype.
tsalo Jun 21, 2024
2c4835e
Generalize change to other suffixes.
tsalo Jun 21, 2024
0ca3021
Fix.
tsalo Jun 24, 2024
59bb2ec
Merge remote-tracking branch 'upstream/master' into reproin-part
tsalo Aug 8, 2024
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
View file
Open in desktop
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
112 changes: 82 additions & 30 deletions heudiconv/heuristics/reproin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,32 +402,61 @@ def infotodict(
run_label: Optional[str] = None # run-
dcm_image_iod_spec: Optional[str] = None
skip_derived = False
for s 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 s.is_derived and not s.is_motion_corrected:
skipped.append(s.series_id)
lgr.debug("Ignoring derived data %s", s.series_id)
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_seqinfo = None
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_seqinfo = 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_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":
s = s._replace(**{f: getattr(s, f).format(**s._asdict())})
curr_seqinfo = curr_seqinfo._replace(
**{f: getattr(curr_seqinfo, f).format(**curr_seqinfo._asdict())}
)

template = None
suffix = ""
# seq = []

# figure out type of image from s.image_info -- just for checking ATM
# 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(s.image_type) > 2:
if len(curr_seqinfo.image_type) > 2:
# https://dicom.innolitics.com/ciods/cr-image/general-image/00080008
# 0 - ORIGINAL/DERIVED
# 1 - PRIMARY/SECONDARY
# 3 - Image IOD specific specialization (optional)
dcm_image_iod_spec = s.image_type[2]
dcm_image_iod_spec = curr_seqinfo.image_type[2]
image_type_datatype = {
# Note: P and M are too generic to make a decision here, could be
# for different datatypes (bold, fmap, etc)
Expand All @@ -443,7 +472,7 @@ def infotodict(

series_info = {} # For please lintian and its friends
for sfield in series_spec_fields:
svalue = getattr(s, sfield)
svalue = getattr(curr_seqinfo, sfield)
series_info = parse_series_spec(svalue)
if series_info: # looks like a valid spec - we are done
series_spec = svalue
Expand All @@ -454,10 +483,10 @@ def infotodict(
if not series_info:
series_spec = None # we cannot know better
lgr.warning(
"Could not determine the series name by looking at " "%s fields",
"Could not determine the series name by looking at %s fields",
", ".join(series_spec_fields),
)
skipped_unknown.append(s.series_id)
skipped_unknown.append(curr_seqinfo.series_id)
continue

if dcm_image_iod_spec and dcm_image_iod_spec.startswith("MIP"):
Expand All @@ -476,14 +505,14 @@ def infotodict(
series_spec,
)

# if s.is_derived:
# if curr_seqinfo.is_derived:
# # Let's for now stash those close to original images
# # TODO: we might want a separate tree for all of this!?
# # so more of a parameter to the create_key
# #datatype += '/derivative'
# # just keep it lower case and without special characters
# # XXXX what for???
# #seq.append(s.series_description.lower())
# #seq.append(curr_seqinfo.series_description.lower())
# prefix = os.path.join('derivatives', 'scanner')
# else:
# prefix = ''
Expand All @@ -493,15 +522,30 @@ def infotodict(
# Figure out the datatype_suffix (BIDS _suffix)
#
# If none was provided -- let's deduce it from the information we find:
# analyze s.protocol_name (series_id is based on it) for full name mapping etc
# analyze curr_seqinfo.protocol_name (series_id is based on it) for full name mapping etc
if not datatype_suffix:
if datatype == "func":
if "_pace_" in series_spec:
datatype_suffix = "pace" # or should it be part of seq-
elif "P" in s.image_type:
datatype_suffix = "phase"
elif "M" in s.image_type:
elif (
"P" in curr_seqinfo.image_type
and not curr_seqinfo.series_description.endswith("_SBRef")
):
datatype_suffix = "bold"
series_info["part"] = "phase"
elif "M" in curr_seqinfo.image_type:
datatype_suffix = "bold"

# if next one is phase fMRI, we should set part to mag
if (
(
next_seqinfo.series_description
== curr_seqinfo.series_description
)
and (next_dcm_image_iod_spec == "P")
and not curr_seqinfo.series_description.endswith("_SBRef")
):
series_info["part"] = "mag"
tsalo marked this conversation as resolved.
Show resolved Hide resolved
else:
# assume bold by default
datatype_suffix = "bold"
Expand All @@ -526,7 +570,7 @@ def infotodict(
# since they are complementary files produced along-side with original
# ones.
#
if s.series_description.endswith("_SBRef"):
if curr_seqinfo.series_description.endswith("_SBRef"):
datatype_suffix = "sbref"

if not datatype_suffix:
Expand All @@ -550,7 +594,10 @@ def infotodict(
# XXX if we have a known earlier study, we need to always
# increase the run counter for phasediff because magnitudes
# were not acquired
if get_study_hash([s]) == "9d148e2a05f782273f6343507733309d":
if (
get_study_hash([curr_seqinfo])
== "9d148e2a05f782273f6343507733309d"
):
current_run += 1
else:
raise RuntimeError(
Expand Down Expand Up @@ -583,10 +630,10 @@ def infotodict(
run_label = None

# yoh: had a wrong assumption
# if s.is_motion_corrected:
# assert s.is_derived, "Motion corrected images must be 'derived'"
# if curr_seqinfo.is_motion_corrected:
# assert curr_seqinfo.is_derived, "Motion corrected images must be 'derived'"

if s.is_motion_corrected and "rec-" in series_info.get("bids", ""):
if curr_seqinfo.is_motion_corrected and "rec-" in series_info.get("bids", ""):
raise NotImplementedError(
"want to add _rec-moco but there is _rec- already"
)
Expand All @@ -611,17 +658,19 @@ def from_series_info(name: str) -> Optional[str]:
from_series_info("acq"),
# But we want to add an indicator in case it was motion corrected
# in the magnet. ref sample /2017/01/03/qa
None if not s.is_motion_corrected else "rec-moco",
None if not curr_seqinfo.is_motion_corrected else "rec-moco",
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]

# # .series_description in case of
# sdesc = s.study_description
# sdesc = curr_seqinfo.study_description
# # temporary aliases for those phantoms which we already collected
# # so we rename them into this
# #MAPPING
Expand All @@ -638,13 +687,16 @@ def from_series_info(name: str) -> Optional[str]:
# https://github.com/nipy/heudiconv/issues/145
outtype: tuple[str, ...]
if (
"_Scout" in s.series_description
"_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()
== curr_seqinfo.protocol_name.lower() + "_setter"
)
or (s.series_description.lower() == s.protocol_name.lower() + "_setter")
):
outtype = ("dicom",)
else:
Expand All @@ -654,7 +706,7 @@ def from_series_info(name: str) -> Optional[str]:
# we wanted ordered dict for consistent demarcation of dups
if template not in info:
info[template] = []
info[template].append(s.series_id)
info[template].append(curr_seqinfo.series_id)

if skipped:
lgr.info("Skipped %d sequences: %s" % (len(skipped), skipped))
Expand Down
Loading