Skip to content
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

Merged
merged 4 commits into from
Oct 20, 2021

Conversation

jdkent
Copy link
Member

@jdkent jdkent commented Oct 12, 2021

Closes #579
ref #576

Changes proposed in this pull request:

  • check if map is group-level
    • skip map if it is not group level
  • check if any maps are found for a contrast
    • raise error if no images are found
  • add test

@jdkent jdkent changed the title [FIX] only download group maps when creating dataset. [FIX] only download group maps when creating dataset and raise error if no images are found for a contrast Oct 13, 2021
Comment on lines +537 to +540
raise ValueError(
f"No images were found for contrast {contrast_name}. "
f"Please check the contrast regular expression: {contrast_regex}"
)
Copy link
Member Author

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.

Copy link
Member

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.

@tsalo
Copy link
Member

tsalo commented Oct 13, 2021

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 flake8-isort. The build_docs error is random and already documented in #418.

I will try to look at the actual code soon.

@tsalo
Copy link
Member

tsalo commented Oct 15, 2021

@jdkent it looks like the isort issue is fixed, although the current cache will have the bad version. If you update info.py or the CircleCI cache strings to trigger a new build, I think CI will pass.

@tsalo
Copy link
Member

tsalo commented Oct 18, 2021

@jdkent all of the downstream jobs from the make_env_py36 job need to have the cache updated too. However, now that #578 is merged, I think you can just rebase and it should pass CI.

@jdkent jdkent force-pushed the fix/neurovault_to_nimare branch from d9c61ec to 50c31ad Compare October 19, 2021 21:34
@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #580 (26c42cd) into main (05bbd89) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
nimare/io.py 95.07% <100.00%> (+0.09%) ⬆️

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 05bbd89...26c42cd. Read the comment docs.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jdkent jdkent merged commit 48fbae0 into neurostuff:main Oct 20, 2021
@tsalo tsalo added the bug Issues noting problems and PRs fixing those problems. label Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues noting problems and PRs fixing those problems.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

single subject maps downloaded from neurovault
2 participants