-
Notifications
You must be signed in to change notification settings - Fork 95
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.
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
[DOC] Improve logging of component table-based manual classification #852
Conversation
Codecov Report
@@ Coverage Diff @@
## main #852 +/- ##
=======================================
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.
|
I'm unable to reproduce the following problem from #848:
First passNote that the selected component is ignored. Manual classificationAll ignored components were reclassified as accepted in this step, via the component table. |
I will try to reproduce it sometime this week. I believe we have another user reporting the same issue in Neurostars. |
@eurunuela it looks like ME-ICA/rica#41 solves the component classification issue, correct? If so, then I think this PR can be reviewed purely as an improvement to documentation/logging. |
Indeed. I'm sorry I didn't have time to report back, it's been a busy evening. As you say, we should improve documentation/logging. |
#855 should fix a related issue on tedana's side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes should be enough.
Should we add something to the docs? And if so, would you rather do that on this PR or open a new one?
I think improving the documentation is a good idea, and I'm happy to incorporate that into this PR. Do you have anything specific in mind? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I don't have any strong opinions on adding documentation here vs a future PR.
My one general concern is how many different things are being tested within some of these integration tests (i.e. manual classification could be a separate test). This is going to create a lot of "fun" when we get to the point of fully reviewing #756 I suspect non-trivial changes to some of these existing tests will be required. It's beyond the scope of this PR, but I'll think about this more, and possibly open up a new issue so that we can start identifying & cleaning up tests that will interface with the modularized decision tree.
Closes #848.
Changes proposed in this pull request: