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

[MRG] load surf data in view_surf #2057

Merged
merged 4 commits into from Jun 13, 2019

Conversation

jeromedockes
Copy link
Member

@jeromedockes jeromedockes commented May 15, 2019

otherwise if it is provided as a file path the function fails e.g.

from nilearn import datasets, plotting
fsaverage = datasets.fetch_surf_fsaverage()
plotting.view_surf(fsaverage['pial_left'], fsaverage['sulc_left'],
                   cmap='Greys', threshold=None, symmetric_cmap=False)

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #2057 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2057      +/-   ##
==========================================
+ Coverage   94.97%   94.97%   +<.01%     
==========================================
  Files         141      141              
  Lines       18254    18256       +2     
==========================================
+ Hits        17336    17338       +2     
  Misses        918      918
Impacted Files Coverage Δ
nilearn/plotting/html_surface.py 100% <100%> (ø) ⬆️
nilearn/plotting/tests/test_html_surface.py 100% <100%> (ø) ⬆️

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 ded4df9...b1932de. Read the comment docs.

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #2057 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2057      +/-   ##
==========================================
+ Coverage   94.98%   94.98%   +<.01%     
==========================================
  Files         141      141              
  Lines       18298    18299       +1     
==========================================
+ Hits        17380    17381       +1     
  Misses        918      918
Impacted Files Coverage Δ
nilearn/plotting/html_surface.py 100% <100%> (ø) ⬆️
nilearn/plotting/tests/test_html_surface.py 100% <100%> (ø) ⬆️

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 2dde3f9...1f6fa47. Read the comment docs.

@kchawla-pi
Copy link
Collaborator

Hey, this is cool. Would you document the reason for this PR in the first comment, so there is a record of the problem or even better file an issue and link it here?

@kchawla-pi
Copy link
Collaborator

I was about to merge it, we should add Fix in whats_new .

@kchawla-pi
Copy link
Collaborator

WHy did none of the tests catch this earlier? Do we have a test that catches this now?

@jeromedockes
Copy link
Member Author

Do we have a test that catches this now?

yes it is added in test_html_surface.py in this pr

@jeromedockes jeromedockes changed the title load surf data in view_surf [MRG] load surf data in view_surf Jun 13, 2019
@jeromedockes
Copy link
Member Author

@kchawla-pi do you have more comments or can we merge?

@kchawla-pi kchawla-pi merged commit 2df00ff into nilearn:master Jun 13, 2019
@jeromedockes jeromedockes deleted the load_surf_data_in_view_surf branch June 14, 2019 14:27
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

2 participants