[ENH] Post-ICA automatic components selection #344

smoia · 2019-06-21T16:33:18Z

This PR was developed during HBM Hackathon 2019.
It's a little script to let the user accept or reject a component in a post-ICA run, based on a input timeseries.
Useful in case a component gets constantly badly categorised (in the scope of a particular study, such as BreathHolding tasks).
tedana has to be run first.

Closes # .

Changes proposed in this pull request:

Adding the file "post_taskcorr.py" in folder workflows.

codecov · 2019-06-21T16:53:54Z

Codecov Report

Merging #344 into master will decrease coverage by 1.66%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
- Coverage   49.22%   47.56%   -1.67%     
==========================================
  Files          39       40       +1     
  Lines        2139     2214      +75     
==========================================
  Hits         1053     1053              
- Misses       1086     1161      +75

Impacted Files	Coverage Δ
tedana/workflows/post_taskcorr.py	`0% <0%> (ø)`

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 2ae6c48...b0b7d57. Read the comment docs.

codecov · 2019-06-21T16:53:54Z

Codecov Report

Merging #344 into master will decrease coverage by 2.86%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #344      +/-   ##
=========================================
- Coverage   82.56%   79.7%   -2.87%     
=========================================
  Files          39      42       +3     
  Lines        2633    2631       -2     
=========================================
- Hits         2174    2097      -77     
- Misses        459     534      +75

Impacted Files	Coverage Δ
tedana/workflows/post_taskcorr.py	`0% <0%> (ø)`
tedana/decay.py	`89.61% <0%> (-5.44%)`	⬇️
tedana/io.py	`95.87% <0%> (-1.19%)`	⬇️
tedana/decomposition/pca.py	`78.09% <0%> (-1%)`	⬇️
tedana/tests/test_integration.py	`95.91% <0%> (-0.38%)`	⬇️
tedana/selection/tedica.py	`88.65% <0%> (-0.16%)`	⬇️
tedana/tests/test_io.py	`91.83% <0%> (ø)`	⬆️
tedana/workflows/t2smap.py	`67.08% <0%> (ø)`	⬆️
tedana/tests/test_selection.py	`100% <0%> (ø)`	⬆️
tedana/tests/test_model_kundu_metrics.py	`100% <0%> (ø)`	⬆️
... and 10 more

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 5e0b3ee...be175c7. Read the comment docs.

jbteves · 2019-06-21T17:13:05Z

@smoia would you mind adding the [ENH] tag to the beginning of your PR title? Thanks so much! This looks great, I will review more closely later.

smoia · 2019-06-27T08:54:27Z

Sorry @jbteves , I saw the message only now.
I probably need to write a bit more comments in the file. Let me know what I can do to improve it and help the merge.

tsalo

It's looking good. I have some small changes I've requested. In addition to those, could you add numpydoc-format docstrings (e.g., add Parameters and Returns sections) and could you add a test file for this workflow? I think it would be good to test good_ts, bad_ts, and both (so three tests).

tedana/workflows/post_taskcorr.py

smoia · 2019-07-02T10:41:57Z

For some reasons I don't get notifications from this PR. Is there a way to get them?
@tsalo, I don't have any file that I can share for the moment. Maybe I can check if one of the test files that are there already needs this trick? Or did I get wrong what you're asking me?

tsalo · 2019-07-04T17:57:15Z

docs/usage.rst

+components.
+It overwrites the original comp_table_ica.txt, adding specific rationale labels (I098 and I099).
+
+.. _Normalised Cross Correlation: https://en.wikipedia.org/wiki/Cross-correlation#Normalized_cross-correlation_(NCC)


You should add the argparse auto-generated documentation here.

That's where I get kind of lost (i.e. I still don't know ho to do it! But I'm studying it). I'm going to look into it tomorrow and hope I can come back on that.

Thank you!

Ok I think I did it!

emdupre · 2019-07-04T18:14:00Z

For some reasons I don't get notifications from this PR. Is there a way to get them?

@smoia -- let's see if somehow you're "unsubscribed" from the conversation. Can you try "unsubscribing" and then "subscribing" as detailed here ?

tsalo · 2019-07-04T18:17:22Z

I think we can hold off on writing tests for now, but perhaps it could fit in among the integration tests. It could even be called after the workflow in one of those tests. Just an integration test to make sure everything runs.

smoia · 2019-07-04T22:13:44Z

For some reasons I don't get notifications from this PR. Is there a way to get them?

@smoia -- let's see if somehow you're "unsubscribed" from the conversation. Can you try "unsubscribing" and then "subscribing" as detailed here ?

Thank you @emdupre , now it works!

smoia · 2019-07-04T22:17:27Z

I think we can hold off on writing tests for now, but perhaps it could fit in among the integration tests. It could even be called after the workflow in one of those tests. Just an integration test to make sure everything runs.

I need to study more this as well. Let's talk about it (if you can), so I get what to do and maybe I can do it with the test files that are there!

rmarkello · 2019-07-05T15:09:13Z

tedana/workflows/post_taskcorr.py

+        Timeseries of MEICA components.
+    ctab_fullpath: :obj:`string`
+        Full path to components table as of tedana output.
+        !!! It will be overwritten !!!


Sorry to just jump in here but maybe it would be nice to make a backup copy of the original components table before overwriting it? Just adding in a line like shutil.copy(ctab_fullpath, ctab_fullpath + '.bak') prior to saving the modified table would be nice!

If this modifies things in a way that a user deems "not great" then they can recover the original comptable without too much hassle rather than having to re-run most of the tedana workflow.

The original classifications and rationales will be retained in "original_classification" and "original_rationale" columns though.

But if I wanted to use e.g., the tools in tedana.viz, those primarily pulls from the "classification" column, right? I guess I can go in and just over-write 'original_classification' to 'classification' if I don't like the taskcorr output...

Alright, sorry about that! 👍 Disregard me just wildly jumping into things 😅

I think it's pretty easy to fix now (since we use csvs), but once we switch to json files for the decompositions it will probably be a good idea to come back to this and to come up with a programmatic solution.

@rmarkello , I might like your idea more.
@tsalo , @emdupre , what do you think?

docs/usage.rst

emdupre · 2019-11-06T15:18:40Z

tedana/workflows/post_taskcorr.py

@@ -0,0 +1,259 @@
+"""
+Quick hack for tedana, from HBM Hackathon 2019.


Can we update the description here to be directly helpful to the user ?

emdupre · 2019-11-06T15:18:50Z

tedana/workflows/post_taskcorr.py

+"""
+Quick hack for tedana, from HBM Hackathon 2019.
+TODO:
+- let user decide wether they want NCC or simple correlation.


Suggested change

- let user decide wether they want NCC or simple correlation.

- let user decide whether they want NCC or simple correlation.

You're still missing this correction.

emdupre · 2019-11-06T15:19:17Z

tedana/workflows/post_taskcorr.py

+Quick hack for tedana, from HBM Hackathon 2019.
+TODO:
+- let user decide wether they want NCC or simple correlation.
+- Better threshold.


Can we clarify this so it's clearer for future developers ?

emdupre · 2019-11-06T15:23:47Z

tedana/workflows/post_taskcorr.py

+    # https://stackoverflow.com/a/43456577
+    optional = parser._action_groups.pop()
+    required = parser.add_argument_group('required arguments')
+    required.add_argument('-td', '--ted-dir',


Let's just call this --dir and -d to make things a little more straightforward.

tedana/workflows/post_taskcorr.py

emdupre · 2019-11-24T19:29:34Z

This is still missing tests !

smoia changed the title ~~Post-ICA automatic components selection~~ [ENH] Post-ICA automatic components selection Jun 27, 2019

tsalo requested changes Jun 27, 2019

View reviewed changes

tsalo reviewed Jul 4, 2019

View reviewed changes

rmarkello reviewed Jul 5, 2019

View reviewed changes

dowdlelt mentioned this pull request Aug 20, 2019

Pain Points of Tedana Meta-Issue #383

Closed

1 task

emdupre reviewed Nov 6, 2019

View reviewed changes

tedana/workflows/post_taskcorr.py Outdated Show resolved Hide resolved

This was referenced Nov 7, 2019

[ENH] Melview #422

Closed

Add ability to denoise data using existing component table #334

Closed

smoia added the enhancement issues describing possible enhancements to the project label Nov 8, 2019

smoia closed this Dec 2, 2019

smoia force-pushed the master branch from 40c60e6 to be175c7 Compare December 2, 2019 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Post-ICA automatic components selection #344

[ENH] Post-ICA automatic components selection #344

smoia commented Jun 21, 2019

codecov bot commented Jun 21, 2019

codecov bot commented Jun 21, 2019 •

edited

Loading

jbteves commented Jun 21, 2019

smoia commented Jun 27, 2019

tsalo left a comment

smoia commented Jul 2, 2019

tsalo Jul 4, 2019

smoia Jul 4, 2019

smoia Jul 5, 2019

emdupre commented Jul 4, 2019

tsalo commented Jul 4, 2019

smoia commented Jul 4, 2019 •

edited

Loading

smoia commented Jul 4, 2019

rmarkello Jul 5, 2019

tsalo Jul 5, 2019

rmarkello Jul 5, 2019 •

edited

Loading

tsalo Jul 5, 2019

smoia Jul 5, 2019

emdupre Nov 6, 2019

emdupre Nov 6, 2019

emdupre Nov 24, 2019

emdupre Nov 6, 2019

emdupre Nov 6, 2019

emdupre commented Nov 24, 2019

		@@ -0,0 +1,259 @@
		"""
		Quick hack for tedana, from HBM Hackathon 2019.

	- let user decide wether they want NCC or simple correlation.
	- let user decide whether they want NCC or simple correlation.

[ENH] Post-ICA automatic components selection #344

[ENH] Post-ICA automatic components selection #344

Conversation

smoia commented Jun 21, 2019

codecov bot commented Jun 21, 2019

Codecov Report

codecov bot commented Jun 21, 2019 • edited Loading

Codecov Report

jbteves commented Jun 21, 2019

smoia commented Jun 27, 2019

tsalo left a comment

Choose a reason for hiding this comment

smoia commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emdupre commented Jul 4, 2019

tsalo commented Jul 4, 2019

smoia commented Jul 4, 2019 • edited Loading

smoia commented Jul 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarkello Jul 5, 2019 • 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

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 commented Nov 24, 2019

codecov bot commented Jun 21, 2019 •

edited

Loading

smoia commented Jul 4, 2019 •

edited

Loading

rmarkello Jul 5, 2019 •

edited

Loading