-
Notifications
You must be signed in to change notification settings - Fork 58
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
[FIX] only download group maps when creating dataset and raise error if no images are found for a contrast #580
Conversation
raise ValueError( | ||
f"No images were found for contrast {contrast_name}. " | ||
f"Please check the contrast regular expression: {contrast_regex}" | ||
) |
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 do not have strong feelings about this being a warning instead, but I chose to raise an error since it could lead to errors down the line if you were trying to run a meta-analysis for a contrast with no images.
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 an error is reasonable. If we see users complain about it a lot, we can revisit, but I agree that downgrading to a warning sounds problematic.
There seems to be a new problem with isort. It also came up in ME-ICA/tedana#815. I haven't looked into it yet, but it might be a matter of pinning the version of I will try to look at the actual code soon. |
@jdkent it looks like the isort issue is fixed, although the current cache will have the bad version. If you update |
d9c61ec
to
50c31ad
Compare
Codecov Report
@@ Coverage Diff @@
## main #580 +/- ##
==========================================
+ Coverage 85.82% 85.83% +0.01%
==========================================
Files 39 39
Lines 4267 4271 +4
==========================================
+ Hits 3662 3666 +4
Misses 605 605
Continue to review full report at Codecov.
|
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!
Closes #579
ref #576
Changes proposed in this pull request: