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 error when colorscale given boolean array #2193

Merged
merged 3 commits into from
Oct 28, 2019

Conversation

jeromedockes
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #2193 into master will decrease coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2193      +/-   ##
==========================================
- Coverage    92.5%   92.34%   -0.16%     
==========================================
  Files         149      149              
  Lines       18870    18872       +2     
  Branches     2299     2299              
==========================================
- Hits        17455    17427      -28     
- Misses        902      926      +24     
- Partials      513      519       +6
Impacted Files Coverage Δ
nilearn/plotting/tests/test_js_plotting_utils.py 97.6% <100%> (-1.6%) ⬇️
nilearn/plotting/js_plotting_utils.py 97.7% <100%> (+0.02%) ⬆️
nilearn/reporting/rm_file.py 26.66% <0%> (-53.34%) ⬇️
nilearn/_utils/testing.py 78.76% <0%> (-4.8%) ⬇️
nilearn/plotting/__init__.py 95.65% <0%> (-4.35%) ⬇️
nilearn/plotting/html_stat_map.py 94.19% <0%> (-2.59%) ⬇️
nilearn/datasets/tests/test_neurovault.py 95.84% <0%> (-0.98%) ⬇️
nilearn/image/image.py 95.58% <0%> (-0.74%) ⬇️

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 342f54f...f2b3941. Read the comment docs.

@@ -62,6 +62,7 @@ def test_colorscale_no_threshold():
assert colors['cmap'].N == 256
assert (colors['norm'].vmax, colors['norm'].vmin) == (13, -13)
assert colors['abs_threshold'] is None
colors = js_plotting_utils.colorscale(cmap, values > 0, .5)
Copy link
Member

Choose a reason for hiding this comment

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

This does not really test the new feature, does it ?

@jeromedockes
Copy link
Member Author

jeromedockes commented Oct 25, 2019 via email

@@ -87,6 +87,7 @@ def colorscale(cmap, values, threshold=None, symmetric_cmap=True,
vmin = 0
if vmax is None:
vmax = abs_values.max()
vmax = float(vmax)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment here, why we are typecasting this to float, so future maintainers will have an easier time understanding it.

@kchawla-pi kchawla-pi merged commit bd27cf5 into nilearn:master Oct 28, 2019
@kchawla-pi
Copy link
Collaborator

Did this need a whats_new?

kchawla-pi added a commit to illdopejake/nilearn that referenced this pull request Nov 9, 2019
…nt_modify_fetch_dev

* 'master' of https://github.com/nilearn/nilearn:
  Verbose doc building to ease tracking of progress & diagnose stalls (nilearn#2203)
  New conda env is created once conda path has been activated
  Conda environment is created for full-builds
  Refactor CircleCI config for reduced redundancy (nilearn#2204)
  Installation should fail on Python < 3.5 (nilearn#2198)
  [MRG] Add get data function (nilearn#2172)
  fix error when colorscale given boolean array (nilearn#2193)
  remove is_valid filter (nilearn#2169)
  Moved new entries to next release
  FIX: marker size issue in plot_connectome nilearn#2185 (nilearn#2186)
  Updated Appveyor status badge
  Run tests on local Windows machines & Azure Pipelines (nilearn#2191)
  Updated requirements list for devs (nilearn#2190)
  Update azure-pipelines.yml for Azure Pipelines
  Release Nilearn 0.6.0 alpha (nilearn#2164)
  Making fetch_localizer_button_task backwards compatibile (nilearn#2182)
  [DOC] Update whats_new to reference nilearn#2013 (Merging of several examples) (nilearn#2183)
kchawla-pi added a commit to pausz/nilearn that referenced this pull request Nov 21, 2019
…smooth-image

* 'master' of https://github.com/nilearn/nilearn: (25 commits)
  Add testing for Nilearn setup & install & Fix the broken dependency installation (nilearn#2201)
  Fix uniform ball cloud test for sklearn >= 0.22 (nilearn#2175)
  MAINT: sklearn Deprecations (nilearn#2219)
  DOC: title in bold
  [DOC] Add note about decreasing memory usage (nilearn#2223)
  Rel 060b0 (nilearn#2208)
  Nilearn 0.6.0b0 release (nilearn#2206)
  Fixed the redundant & missing test case in merged PR nilearn#2035 (nilearn#2205)
  Modify fetch_development_fmri to fetch adults or children (nilearn#2035)
  Verbose doc building to ease tracking of progress & diagnose stalls (nilearn#2203)
  New conda env is created once conda path has been activated
  Conda environment is created for full-builds
  Refactor CircleCI config for reduced redundancy (nilearn#2204)
  Installation should fail on Python < 3.5 (nilearn#2198)
  [MRG] Add get data function (nilearn#2172)
  fix error when colorscale given boolean array (nilearn#2193)
  remove is_valid filter (nilearn#2169)
  Moved new entries to next release
  FIX: marker size issue in plot_connectome nilearn#2185 (nilearn#2186)
  Updated Appveyor status badge
  ...
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