Skip to content

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

Open
tsalo wants to merge 6 commits into
nipy:masterfrom
tsalo:reproin-part
Open

Replace deprecated "_phase" suffix in reproin heuristic with "part-phase" entity#770
tsalo wants to merge 6 commits into
nipy:masterfrom
tsalo:reproin-part

Conversation

@tsalo

@tsalo tsalo commented Jun 21, 2024

Copy link
Copy Markdown
Member

Closes #769. This is working on DICOMs associated with https://openneuro.org/datasets/ds005250.

Changes proposed:

  • Track previous and next sequence info in ReproIn's infotodict.
  • Rename s variable to curr_seqinfo for improved clarity. Moved to Rename s variable to curr_seqinfo in reproin heuristic #779.
  • For non-SBRef, non-fieldmap scans, set "part" entity to "phase" for phase data instead of setting the suffix to "phase".
  • For magnitude non-SBRef, non-fieldmap scans, check if the next sequence has the same series_description and is a phase scan. If it is, set "part" to "mag".

@tsalo tsalo added the BIDS label Jun 21, 2024
Comment thread heudiconv/heuristics/reproin.py Outdated
@tsalo tsalo marked this pull request as draft June 21, 2024 16:10
@tsalo tsalo marked this pull request as ready for review June 24, 2024 14:34
@codecov

codecov Bot commented Jun 24, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.15%. Comparing base (a0a3635) to head (59bb2ec).
⚠️ Report is 112 commits behind head on master.

Files with missing lines Patch % Lines
heudiconv/heuristics/reproin.py 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #770      +/-   ##
==========================================
+ Coverage   82.08%   82.15%   +0.06%     
==========================================
  Files          42       42              
  Lines        4215     4230      +15     
==========================================
+ Hits         3460     3475      +15     
  Misses        755      755              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsalo tsalo requested a review from yarikoptic June 24, 2024 15:00
@yarikoptic

Copy link
Copy Markdown
Member

Rename s variable to curr_seqinfo for improved clarity.

yeah, but complicates review a little due to diff now including changes I can ignore or not and need to decide when looking...

Could you submit a separate PR with that typo fix and such a rename (so no functionality change) and then let's improve this one -- yet to grasp on all the changes, and as we do not have any nice testing for reproin heuristic, I wonder how we could add one :-/ I would really be shy to accept without some way to test. Locally I could do using smth like https://github.com/nipy/heudiconv/blob/master/utils/test-compare-two-versions.sh but could we do better?!

@tsalo

tsalo commented Aug 8, 2024

Copy link
Copy Markdown
Member Author

👍 I've opened #779 with the variable name change.

@yarikoptic yarikoptic added the minor Increment the minor version when merged label Oct 1, 2024

@yarikoptic yarikoptic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! some feedback to improve it.

if "P" in curr_seqinfo.image_type:
series_info["part"] = "phase"
elif "M" in curr_seqinfo.image_type:
# if next one is phase from same scan, we should set part to mag

Copy link
Copy Markdown
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"?

"_SBRef"
):
if "P" in curr_seqinfo.image_type:
series_info["part"] = "phase"

Copy link
Copy Markdown
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 ;-)

and datatype_suffix
and datatype_suffix.startswith("scout")
and filename_suffix_parts[-1]
and filename_suffix_parts[-1].startswith("scout")

Copy link
Copy Markdown
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?


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

Copy link
Copy Markdown
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.

@yarikoptic

Copy link
Copy Markdown
Member

@tsalo -- ping on this. Would you happen to get time to address comments?

@tsalo

tsalo commented Oct 30, 2024

Copy link
Copy Markdown
Member Author

Sorry, I've been really busy. I might have time in the next couple of weeks though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BIDS minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ReproIn heuristic uses phase suffix instead of part-phase

2 participants