Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

this PR fixes the load_curvature() function of the _Brain object.

master PR
2020-02-05_1920x1080 2020-02-05_1920x1080

@GuillaumeFavelier GuillaumeFavelier self-assigned this Feb 5, 2020
@GuillaumeFavelier GuillaumeFavelier changed the title Update read_curvature FIX,VIZ: Update read_curvature Feb 5, 2020
mne/surface.py Outdated

def read_curvature(filepath):
def read_curvature(filepath, binary=True):
"""Load in curavature values from the ?h.curv file."""
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised there is no docstring in this funciton and CIs don't complain. Can you add it?

Copy link
Member

Choose a reason for hiding this comment

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

It's not checked because mne.surface is not in the public_modules list in mne/tests/test_docstring_parameters.py list. @GuillaumeFavelier feel free to add it. You might then need to fix some docstrings and/or add entries to docstring_ignores

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #7289 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7289   +/-   ##
=======================================
  Coverage   89.80%   89.80%           
=======================================
  Files         447      447           
  Lines       80572    80572           
  Branches    12873    12873           
=======================================
  Hits        72360    72360           
  Misses       5385     5385           
  Partials     2827     2827           

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

Besides LGTM

Input path to the .curv file.
binary: bool
Specify if the output array is to hold binary values. Defaults to True.

Copy link
Member

Choose a reason for hiding this comment

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

Returns

is missing

mne/surface.py Outdated

Parameters
----------

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

@GuillaumeFavelier
Copy link
Contributor Author

It's not checked because mne.surface is not in the public_modules list in mne/tests/test_docstring_parameters.py list.

This seems beyond the scope of this PR, I noted it in #7162

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Feb 5, 2020
86 tasks
@larsoner larsoner merged commit 33f224d into mne-tools:master Feb 5, 2020
@larsoner
Copy link
Member

larsoner commented Feb 5, 2020

Thanks @GuillaumeFavelier

@GuillaumeFavelier GuillaumeFavelier deleted the fix_read_curvature branch February 5, 2020 16:07
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Update read_curvature

* Improve docstring

* Fix docstring
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Update read_curvature

* Improve docstring

* Fix docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants