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] link terms to glossary #4038
Conversation
👋 @Remi-Gau Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #4038 +/- ##
=======================================
Coverage 91.56% 91.56%
=======================================
Files 143 143
Lines 16057 16057
Branches 3332 3332
=======================================
Hits 14703 14703
Misses 811 811
Partials 543 543
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
And of course I messed that conflict resolution... Well... Will reset and try again... |
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.
Thanks for resolving that, I know it came from my PR :) LGTM!
Yeah I think I actually need to redo the merge conflict resolution. |
Ah ok, the diff and merge look fine to me but given all the affected files it's hard to tell so I'll let you handle it |
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.
Looks great. Just outline a few occurrences that may have been forgotten.
@@ -141,7 +141,7 @@ i.e. values of ``z`` larger than the :math:`(1-\alpha)`-quantile of the standard | |||
The first idea that one might think of is to take a much smaller :math:`\alpha`: | |||
for instance, if we take, :math:`\alpha=\frac{0.05}{n\_voxels}` | |||
then the expected number of false discoveries is only about 0.05, meaning | |||
that there is a 5% chance that a truly inactive voxel is declared active. | |||
that there is a 5% chance that a truly inactive :term:`voxel` is declared active. |
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.
Unsure whether we really need to track all occurrences of 'voxels' ;-)
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.
Trust me: I left some out. 😉
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, thx.
Co-authored-by: bthirion <bertrand.thirion@inria.fr>
Changes proposed in this pull request: