[REF] Rename T1c to "minimum image regression" #609

tsalo · 2020-09-24T17:29:34Z

Closes #605.

Changes proposed in this pull request:

Rename function name, variables, and output files for T1c global signal regression to match new designation as "minimum image regression."
Change multiline string-based comments to regular comments.
Clean up some steps (e.g., z-scoring of arrays) in the function.

codecov · 2020-09-24T18:36:29Z

Codecov Report

Merging #609 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #609   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files          26       26           
  Lines        1965     1965           
=======================================
  Hits         1838     1838           
  Misses        127      127

Impacted Files	Coverage Δ
tedana/gscontrol.py	`100.00% <100.00%> (ø)`
tedana/workflows/tedana.py	`90.11% <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 b0b81bc...5568a28. Read the comment docs.

jbteves · 2020-10-05T14:05:28Z

tedana/gscontrol.py

-    acc = comptable[comptable.classification == 'accepted'].index.values
-    ign = comptable[comptable.classification == 'ignored'].index.values
+    acc = comptable[comptable.classification == "accepted"].index.values
+    ign = comptable[comptable.classification == "ignored"].index.values
    not_ign = sorted(np.setdiff1d(all_comps, ign))

    optcom_masked = optcom_ts[mask, :]
    optcom_mu = optcom_masked.mean(axis=-1)[:, np.newaxis]


If we're naming everything based on stat names, shouldn'lt we make this optcom_mean instead of optcom_mu? I figure since we have other style changes in this PR, this would be a good one to change this for.

jbteves

Only one slight change requested, otherwise looks good!

jbteves · 2020-10-05T14:07:16Z

tedana/gscontrol.py

-    bold_ts = np.dot(cbetas[:, acc], mmix[:, acc].T)
-    t1_map = bold_ts.min(axis=-1)
+    # Build BOLD time series without amplitudes, and save T1-like effect
+    mehk_ts = np.dot(comp_pes[:, acc], mmix[:, acc].T)


It's a little counter-intuitive to me to change this from "bold_ts" to "mehk_ts" (mid-elbow, high-kappa?) but leaving the comment the same. I'd recommend either changing the comment or reverting the variable name change.

You're totally right. I will improve the comment. I switched to MEHK (multi-echo high kappa) because "BOLD" is so unclear when we're dealing with multi-echo data.

Thanks, I think that helps !! On the same point : can we write out MEHK here ?

emdupre

Thanks, @tsalo ! I think this looks great.
I had a few small clarifications / questions.

emdupre · 2020-10-14T18:09:40Z

tedana/gscontrol.py

 from scipy import stats
 from scipy.special import lpmv

 from tedana import io, utils

 LGR = logging.getLogger(__name__)
-RepLGR = logging.getLogger('REPORT')
-RefLGR = logging.getLogger('REFERENCES')
+RepLGR = logging.getLogger("REPORT")


Was this just for style ?

Yeah, I ran black and isort on the file and then only kept changes from the top of the file and the minimum_image_regression function.

Scratch that- I don't think I ran isort. But I did run black.

tedana/gscontrol.py

emdupre · 2020-10-14T18:11:23Z

tedana/gscontrol.py

-    bold_ts = np.dot(cbetas[:, acc], mmix[:, acc].T)
-    t1_map = bold_ts.min(axis=-1)
+    # Build BOLD time series without amplitudes, and save T1-like effect
+    mehk_ts = np.dot(comp_pes[:, acc], mmix[:, acc].T)


Thanks, I think that helps !! On the same point : can we write out MEHK here ?

emdupre · 2020-10-14T18:12:41Z

tedana/gscontrol.py

-    t1_map = bold_ts.min(axis=-1)
+    # Build time series of just BOLD-like components (i.e., MEHK) and save T1-like effect
+    mehk_ts = np.dot(comp_pes[:, acc], mmix[:, acc].T)
+    t1_map = mehk_ts.min(axis=-1)  # map of T1-like effect


I know that this is a bigger question, but if we can maybe mention in the docstring that this is how we derive the T1-like effect ? Or should we change this comment / variable to reference the "minimum image" ?

I think adding to the docstring would be better, because it'll also show up in the API docs.

I've improved the docstring.

emdupre · 2020-10-14T18:13:29Z

tedana/gscontrol.py

-    io.filewrite(utils.unmask(cbetas_norm[:, 2:], mask),
-                 op.join(out_dir, 'betas_hik_OC_T1c'), ref_img)
-    np.savetxt(op.join(out_dir, 'meica_mix_T1c.1D'), mmixnogs)
+    # Find the global signal based on the T1-like effect


I'm not sure why the diff is splitting like this, but just to confirm: None of the functionality is changed here, no ?

(From my look-through I'd think not, but wanted to double-check !

Yeah, that's weird... You're correct, though, that the functionality shouldn't change.

It's splitting like that because of the change from """ -> #
They're all so close together that the heuristic is counting it as big text blobs since it's multi-line.

jbteves

This looks good, thanks @tsalo !

tsalo · 2020-10-19T13:51:26Z

@jbteves I hate to discard your review, but I improved the docstring and added a reference to make things clearer. I'd love to know what you and @emdupre think of the new section in Notes.

jbteves · 2020-10-19T13:56:25Z

I think it's a good addition, thanks!

emdupre

This looks great !! I had one small suggestion, but it's not worth holding up this PR over. Happy to see this in 👍

Co-authored-by: Elizabeth DuPre <emd222@cornell.edu>

tsalo · 2020-10-19T22:33:05Z

Thanks @emdupre and @jbteves. I'll wait until the CI passes and then I'll merge. No need to re-review.

tsalo added 3 commits September 24, 2020 12:44

Initial renaming.

39963a5

Update MIR function.

42567e0

Change the name elsewhere.

8a875ec

tsalo requested review from dowdlelt and jbteves October 3, 2020 15:21

jbteves reviewed Oct 5, 2020

View reviewed changes

jbteves requested changes Oct 5, 2020

View reviewed changes

Address review.

c557f80

tsalo requested a review from jbteves October 13, 2020 21:56

emdupre reviewed Oct 14, 2020

View reviewed changes

emdupre mentioned this pull request Oct 14, 2020

Topics for the October 2020 Dev Call #611

Closed

jbteves previously approved these changes Oct 19, 2020

View reviewed changes

Improve docstring and add reference.

c15c715

tsalo dismissed jbteves’s stale review via c15c715 October 19, 2020 13:50

Update gscontrol.py

3e802b2

jbteves previously approved these changes Oct 19, 2020

View reviewed changes

tsalo requested a review from emdupre October 19, 2020 15:50

emdupre previously approved these changes Oct 19, 2020

View reviewed changes

Update tedana/gscontrol.py

5568a28

Co-authored-by: Elizabeth DuPre <emd222@cornell.edu>

tsalo dismissed stale reviews from emdupre and jbteves via 5568a28 October 19, 2020 22:32

tsalo merged commit 005e9d9 into ME-ICA:master Oct 19, 2020

tsalo deleted the ref/mir-t1c branch October 19, 2020 22:34

jbteves mentioned this pull request Feb 5, 2021

Post-release Newsletter for February 2021 #673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] Rename T1c to "minimum image regression" #609

[REF] Rename T1c to "minimum image regression" #609

tsalo commented Sep 24, 2020

codecov bot commented Sep 24, 2020 •

edited

Loading

jbteves Oct 5, 2020

jbteves left a comment

jbteves Oct 5, 2020

tsalo Oct 12, 2020

emdupre Oct 14, 2020

emdupre left a comment

emdupre Oct 14, 2020

tsalo Oct 14, 2020

tsalo Oct 14, 2020

emdupre Oct 14, 2020

emdupre Oct 14, 2020

jbteves Oct 14, 2020

tsalo Oct 19, 2020

emdupre Oct 14, 2020 •

edited

Loading

emdupre Oct 14, 2020

tsalo Oct 14, 2020

jbteves Oct 19, 2020

jbteves left a comment

tsalo commented Oct 19, 2020

jbteves commented Oct 19, 2020

emdupre left a comment

tsalo commented Oct 19, 2020

[REF] Rename T1c to "minimum image regression" #609

[REF] Rename T1c to "minimum image regression" #609

Conversation

tsalo commented Sep 24, 2020

codecov bot commented Sep 24, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jbteves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emdupre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emdupre Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbteves left a comment

Choose a reason for hiding this comment

tsalo commented Oct 19, 2020

jbteves commented Oct 19, 2020

emdupre left a comment

Choose a reason for hiding this comment

tsalo commented Oct 19, 2020

codecov bot commented Sep 24, 2020 •

edited

Loading

emdupre Oct 14, 2020 •

edited

Loading