-
Notifications
You must be signed in to change notification settings - Fork 30
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 afni contrast bool #266
Fix afni contrast bool #266
Conversation
Hello @Shotgunosine, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2020-11-20 23:28:02 UTC |
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.
No obvious problems with the logic, but here are some simplifications.
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Tests passing. Is this branch working for you? Do you have a test model snippet that can exercise this? |
Double checking the changes now. here's a slightly tweaked ds003_model001 that would provoke the bug if it was still present:
As for that line that's not getting tested, I don't think that case should normally come up, but I didn't want the counts to get out of wack if it did come up for some reason. |
It's working locally for me, I'd be ok if you wanted to go ahead and merge it. |
Sounds good. Looks like we're covering the cases. Would be good to set up more rigorous tests when we have time. |
closes #265