[FIX] The rationale column of comptable gets updated when no manacc is given #855
Conversation
Codecov Report
@@ Coverage Diff @@
## main #855 +/- ##
=======================================
Coverage 93.27% 93.28%
=======================================
Files 27 27
Lines 2217 2218 +1
=======================================
+ Hits 2068 2069 +1
Misses 149 149
Continue to review full report at Codecov.
|
tedana/workflows/tedana.py
Outdated
| manacc = comptable.index[comptable["classification"] == "accepted"].tolist() | ||
| comptable, metric_metadata = selection.manual_selection(comptable, acc=manacc) |
There was a problem hiding this comment.
I don't think we can assume that an updated component table reflects manual classification. For example, if we get AROMA working, we could run the same data through both tedana and AROMA to produce merged classifications derived from both workflows.
There was a problem hiding this comment.
Okay, would you be happy if we updated the rationale column instead?
There was a problem hiding this comment.
Hopefully once we have the decision tree refactor done and have switched from rationales to the messages we plan to use, we can just require classification tools to update that information. In the mean time, I think your empty string solution is great. On RICA's side, could you update the rationale column as well?
There was a problem hiding this comment.
I agree things will be different soon, but users need a fix now.
Re Rica: I could make Rica update the rationale to say "manual" or something similar.
There was a problem hiding this comment.
Yes, if Rica could updated the rationale for any component with an updated classification, that would be optimal. "Manual" or "I001" would both work well.
There was a problem hiding this comment.
Will work on adding that to Rica, maybe while I do some scanning tomorrow.
Thanks for the feedback Taylor!
There was a problem hiding this comment.
PR #42 in Rica adds this functionality.
Thank you Dan! Just wanna make sure users using the current version don't run into issues :) Edit: merging now. |
Closes #854 .
Changes proposed in this pull request:
ctabis Rica's by looking at the column names.comptablewithselection.manual_selection(comptable, acc=manacc).