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

fixed code + added test #3137

Merged
merged 4 commits into from Jan 24, 2022
Merged

fixed code + added test #3137

merged 4 commits into from Jan 24, 2022

Conversation

bthirion
Copy link
Member

Closes # .

Changes proposed in this pull request:

@github-actions
Copy link
Contributor

👋 @bthirion 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.

  • PR has an interpretable title.
  • PR links to Github issue with mention "Closes #XXXX"
  • Code is PEP8-compliant.
  • (Bug fixes) There is at least one test that would fail under the original bug conditions.
  • (New features) There is at least one unit test per new function / class.
  • (New features) The new feature is demoed in at least one relevant example.

We will review it as quick as possible, feel free to ping us with questions if needed.

@bthirion
Copy link
Member Author

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #3137 (1369b22) into main (c123b61) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
- Coverage   90.18%   90.16%   -0.02%     
==========================================
  Files         119      121       +2     
  Lines       14442    14445       +3     
  Branches     2957     2958       +1     
==========================================
  Hits        13024    13024              
- Misses        848      851       +3     
  Partials      570      570              
Impacted Files Coverage Δ
nilearn/glm/thresholding.py 95.69% <100.00%> (ø)
nilearn/reporting/glm_reporter.py 93.77% <0.00%> (-1.78%) ⬇️
nilearn/image/image.py 95.61% <0.00%> (-0.63%) ⬇️
nilearn/_utils/glm.py 89.56% <0.00%> (-0.31%) ⬇️
..._testing/_glm_reporter_visual_inspection_suite_.py 87.50% <0.00%> (ø)
nilearn/interfaces/bids.py 94.28% <0.00%> (ø)
nilearn/interfaces/fsl.py 80.00% <0.00%> (ø)
nilearn/glm/first_level/first_level.py 88.54% <0.00%> (+0.03%) ⬆️
nilearn/reporting/_get_clusters_table.py 100.00% <0.00%> (+3.12%) ⬆️

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 c123b61...1369b22. Read the comment docs.

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
Maybe you can add a whatsnew entry?

@NicolasGensollen NicolasGensollen added this to the 0.9 milestone Jan 21, 2022
Copy link
Member

@jeromedockes jeromedockes left a comment

Choose a reason for hiding this comment

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

great!

@@ -68,6 +68,8 @@ Fixes
index (See PR `#3078 <https://github.com/nilearn/nilearn/issues/3078>`_).
- Convert reference in `nilearn/regions/region_extractor.py` to use footcite / footbibliography.
(See issue `#2787 <https://github.com/nilearn/nilearn/issues/2787>`_ and PR `#3111 <https://github.com/nilearn/nilearn/pull/3111>`_).
- Computation of Benjamini-Hocheberg threshold fixed in `nilearn/glm/thresholding.py` function (see issue ``#2879 <https://github.com/nilearn/nilearn/issues/2879>`_ and PR `#3137 <https://github.com/nilearn/nilearn/pull/3137>`_`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Computation of Benjamini-Hocheberg threshold fixed in `nilearn/glm/thresholding.py` function (see issue ``#2879 <https://github.com/nilearn/nilearn/issues/2879>`_ and PR `#3137 <https://github.com/nilearn/nilearn/pull/3137>`_`)
- Computation of Benjamini-Hocheberg threshold fixed in `nilearn/glm/thresholding.py` function (see issue `#2879 <https://github.com/nilearn/nilearn/issues/2879>`_ and PR `#3137 <https://github.com/nilearn/nilearn/pull/3137>`_)

@NicolasGensollen
Copy link
Member

Thanks, merging...

@NicolasGensollen NicolasGensollen merged commit 4e9ef3b into nilearn:main Jan 24, 2022
achamma723 pushed a commit to achamma723/nilearn that referenced this pull request Jan 26, 2022
* fixed code + added test

* pep8

* Added to whatsnew

* string fix
NicolasGensollen added a commit that referenced this pull request Jan 28, 2022
* Repalce numpy with pandas

* [MAINT] Fix new typos found by codespell (#3101)

Fixes c87847b / #3003.

* Fix pep8 + destrieux

* Fix pep8

* Merge docs

* continue work (unfinished)

* Iter

* Fix test

* Fix other test

* Iter

* Add warnings

* Fix PEP8

* Address Jerome's reviews

* Fix PEP8

* convert to lower case in fetch_atlas_difumo

* [circle full] Add whatsnew.

* Fix

* [circle full] remove deprecation warning from example

* Fix examples

* [ENH] Add `cbar_tick_format` to plotting functions (#2859)

* enable cbar_tick_format in BaseSlicer

* show in examples

* Add whatsnew entry

* [circle full] test full build

* Fix doctrings and kwarg for colorbar and cbar_tick_format

* pep8

* Re-add tests and fix rebase issues

* Fix PEP8

* [circle full] request full build

* plot_roi cbar tick format defaults to integers

* escape %

* [circle full] request full build

* Rename private functions of `permuted_least_squares` to start with '_' (#3131)

* rename functions

* update test

* Fix PEP8

* [MAINT] Update git protocol in CircleCI fetch step (#3124)

* update protocol

* try force cleaning

* try clearing cache

* [MAINT] Scipy deprecation warning in RegionExtractor (#3130)

atol='legacy' option in `scipy.sparse.linalg.cg` to avoid deprecation warning

* [MAINT] Remove deprecated `sessions` and `sample_mask` attributes of `NiftiMasker` (#3133)

* remove deprecated sessions and sample_mask

* Update tests

* Update doctest

* [circle full] add whatsnew entry

* [circle full] fix

* [MAINT] Remove old workaround (#3092)

* [ENH] Move FSL- and BIDS-related functions to interfaces module (#3126)

* Move BIDS and FSL-related functions to the interfaces module.

* Update associated imports across the library.

* Add migrated functions to the reference page.

* Add entries to whatsnew page. Will need to update with PR link.

* Move tests for new functions into appropriate files.

* Update the pull request number.

* [FIX] Fix links in whats_new (#3139)

* [circle full] fix links

* [circle full] fix wrong glossary entry

* [FIX] fixed code + added test (#3137)

* fixed code + added test

* pep8

* Added to whatsnew

* string fix

* [ENH] Refactor `plot_matrix` (#3001)

* Refactor plot_matrix

* Improve docstrings

* Fix PEP8

* [circle full] request full build

* Add tests for plot_matrix and refactor

* Fix PEP8

* Add missing test

* [circle full] request full build

* [circle full] Fix method reference

* [circle full] Avoid hardcoded parameters.

* Update nilearn/plotting/matrix_plotting.py

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

* [FIX] replace interpreter call (#3136)

The python command is not guaranteed to exist. On some systems it refers to old python 2.

See https://www.python.org/dev/peps/pep-0394/

* [FIX] `FirstLevelModel` signal_scaling  (#3135)

* Add failing test

* get rid of scaling_axis attribute

* deprecate scaling_axis

* [circle full] add whatsnew entry

* Fix

* [ENH] Include Hierarchical KMeans in regions.Parcellations (#2282)

* Including Hierarchical Kmeans in doc and example

* Adding hierarchical k_means and make it available in parcellations

* test hierarchical_kmeans and test Parcellation using it

* fix incompatible apis of fit() and transform() methods

* Improve docstring and example layout

* "Upgrade to get_data function"

* "changing nose function to pytest function as the rest of the library evolved"

* "solving sklearn.utils.testing deprecation"

* "improve flake8"

* "flake8, missing import, adding test exception"

* Apply suggestions from code review

Including B.T. suggestions

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

* "adding tests for 'scaling' and warning ; fixed scaling bug"

* "test_refactoring"

* "flake8 fixes"

* "clarify example, fix PEP8"

* Apply suggestions from code review

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

* "apply suggestions from N.G. review"

* Apply suggestions from code review

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

* "apply B.T refactoring"

* "flake8"

* "flake8"

* "codespell"

* "enforcing final number of clusters is the one asked by the user"

* "solving bugging test_case"

* "solving edge case"

* "missing float casting"

* "check that adjusted clusters are int"

* "flake8 fix"

* Update nilearn/regions/hierarchical_kmeans_clustering.py

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

* Update examples/03_connectivity/plot_data_driven_parcellations.py

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

* Update examples/03_connectivity/plot_data_driven_parcellations.py

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

* "fix code review comments"

* 'fix bug introduced by review suggestion'

* Update nilearn/regions/hierarchical_kmeans_clustering.py

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

* Update nilearn/regions/hierarchical_kmeans_clustering.py

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

* Update nilearn/regions/hierarchical_kmeans_clustering.py

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

* Update nilearn/regions/hierarchical_kmeans_clustering.py

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

* "apply code review suggestions"

* "add whats_new"

* fixing docstring

* addressing comments of jdockes

* trying to fix pep8 violations

* simplifying hierarchical k-means avoiding useless checks

* pep8

* Addressing other comments by jdockes

Co-authored-by: BAZEILLE Thomas <thomas.bazeille@inria.fr>
Co-authored-by: bthirion <bertrand.thirion@inria.fr>
Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

* [DOC] Fix wrong whats_new entry (#3142)

* [FIX] Fix hommel value computation (#3109)

* added deploy key

* clean deploy key

* clean deploy key

* clean cricreci.yml

* clean circleci.yml

* clean circleci.yml

* clean circleci.yml

* fixed Hommel function + added test

* pep8 fixes

* pep8 fixes

* pep8 fixes

* correct use of assert

* revert unrelated change

* minor fixed on hommel value computation

* addedwhatsnew entry

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

* [DOC] Refactor change logs (#3049)

* Add gh_substitutions

* Add links to contributor profiles (unfinished)

* Use contributor links in AUTHORS

* Break and refactor changelog (unfinished...)

* [circle full] Fix typo and run full build in strict mode.

* Add more links

* iter

* [circle full] request full build

* Add more links again

* [circle full] Iter.

* Update AUTHORS.rst

* gh_role --> _gh_role

* [circle full] Add fundings.

* [circle full] Fix wrong ref.

* [circle full] update niconnect link

* [circle full] fix ref

* fix refs: input_data -> maskers

* [circle full] request full build

* [circle full] fix old ref to fetch_cobre

* [circle full] fix broken ref

* [circle full] Fix error in rebase

* [circle full] fix whats_new bug

* [MAINT] Bump dependencies for release `0.9.0` (#3143)

* bump dependencies

* [circle full] Add whatsnew.

* Jerome's review

* remove warning

* [INFRA] Add tests min requirements with Matplotlib (#3144)

* Add tests min req with matplotlib

* remove useless variable

* [circle full] update README

* 2021 -> 2022 (#3146)

* Revert "[MAINT] Bump dependencies for release `0.9.0` (#3143)"

This reverts commit 26de3fe.

* Revert "[INFRA] Add tests min requirements with Matplotlib (#3144)"

This reverts commit 27e00f0.

* Revert "2021 -> 2022 (#3146)"

This reverts commit 304e782.

Co-authored-by: Dimitri Papadopoulos Orfanos <3234522+DimitriPapadopoulos@users.noreply.github.com>
Co-authored-by: NicolasGensollen <nicolas.gensollen@gmail.com>
Co-authored-by: Hao-Ting Wang <htwangtw@gmail.com>
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
Co-authored-by: bthirion <bertrand.thirion@inria.fr>
Co-authored-by: Ben Greiner <code@bnavigator.de>
Co-authored-by: ThomasBaz <6551732+thomasbazeille@users.noreply.github.com>
Co-authored-by: BAZEILLE Thomas <thomas.bazeille@inria.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants