Skip to content

ENH Brain.add_label(): use the label.hemi and label.color attributes #92

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

Merged
merged 3 commits into from
Feb 3, 2014

Conversation

christianbrodbeck
Copy link
Collaborator

When plotting Label instance, use the label.hemi attribute

@@ -1052,7 +1052,7 @@ def add_label(self, label, color="crimson", alpha=1, scalar_thresh=None,
else:
# try to extract parameters from label instance
try:
lhemi = label.hemi
hemi = label.hemi
Copy link
Contributor

Choose a reason for hiding this comment

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

Can hemi be None coming from mne-python? I can't remember...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, mne.read_label() contains

    if basename.endswith('lh.label') or basename.startswith('lh.'):
        hemi = 'lh'
    elif basename.endswith('rh.label') or basename.startswith('rh.'):
        hemi = 'rh'
    else:
        raise ValueError('Cannot find which hemisphere it is. File should end'
                         ' with lh.label or rh.label')

and Label.__init__()

        if not isinstance(hemi, string_types):
            raise ValueError('hemi must be a string, not %s' % type(hemi))

(wouldn't it make more sense to check it's in ('lh', 'rh')?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I think the current check (lh.label end or lh. start) was designed to follow a standard naming mechanism, likely because it's what freesurfer expects / can handle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant "(wouldn't it make more sense to check it's in ('lh', 'rh')?)" applying to Label.__init__()

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure where you mean... Label.__init__() in mne-python?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, Label.__init__() in mne-python has the lines

        if not isinstance(hemi, string_types):
            raise ValueError('hemi must be a string, not %s' % type(hemi))

And I was wondering why not checking that it's "lh" or "rh"

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, maybe because we support adding the attribute later manually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But then wouldn't we allow None instead of a random string?

On Jan 30, 2014, at 8:09 PM, Eric89GXL notifications@github.com wrote:

In surfer/viz.py:

@@ -1052,7 +1052,7 @@ def add_label(self, label, color="crimson", alpha=1, scalar_thresh=None,
else:
# try to extract parameters from label instance
try:

  •            lhemi = label.hemi
    
  •            hemi = label.hemi
    
    Not sure, maybe because we support adding the attribute later manually?


Reply to this email directly or view it on GitHub.

@christianbrodbeck
Copy link
Collaborator Author

I took the opportunity to also use the new mne-python label.color attribute (mne-tools/mne-python#1111)

an object with attributes "hemi", "vertices", "name",
and (if scalar_thresh is not None) "values".
an object with attributes "hemi", "vertices", "name", and
optionally "color" and "values" (if scalar_thresh is not None).
color : matplotlib-style color
Copy link
Contributor

Choose a reason for hiding this comment

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

matplotlib-style color | None

@agramfort
Copy link
Contributor

besides my nitpick LGTM

@christianbrodbeck
Copy link
Collaborator Author

Updated the DOC as suggested by @agramfort

agramfort added a commit that referenced this pull request Feb 3, 2014
ENH Brain.add_label(): use the label.hemi and label.color attributes
@agramfort agramfort merged commit 9db8414 into nipy:master Feb 3, 2014
@agramfort
Copy link
Contributor

thx

@christianbrodbeck christianbrodbeck deleted the label.hemi branch February 3, 2014 22:59
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.

3 participants