Skip to content

[MRG] Mne watershed bem #2194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 40 commits into from
Jun 20, 2015
Merged

[MRG] Mne watershed bem #2194

merged 40 commits into from
Jun 20, 2015

Conversation

agramfort
Copy link
Member

taking over #1885

see my last commit.

@agramfort agramfort mentioned this pull request Jun 10, 2015
Closed
larsoner
larsoner reviewed Jun 10, 2015
View reviewed changes
mne/bem.py
# Create BEM surfaces

@verbose
def make_watershed_bem(subject=None, subjects_dir=None, overwrite=False,
Copy link
Member

Choose a reason for hiding this comment

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

if subject is required it should not default to None

Copy link
Member

Choose a reason for hiding this comment

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

also, please add this function to the docs, I see no edits to doc/source

@larsoner
Copy link
Member

Other than my minor gripes LGTM

@agramfort
Copy link
Member Author

@Eric89GXL please take over. We need to add the test you suggested for sample-head.fif

also I get a weird error when running `mne watershed_bem -s sample -o``

mne_convert_surface tells me:

mne_convert_surface version 1.14 compiled at Mar  4 2015 12:21:23

surf input file       : sample_brain_surface
surf output file      : sample_brain_surface

Triangle file : created by alex on Wed Jun 10 06:03:34 2015 nvert = 10242 ntri = 20480
valid = 1  # volume info valid
filename = /Users/alex/work/data/subjects/sample/mri/T1.mgz
volume = 256 256 256
voxelsize = 1.0000 1.0000 1.0000
xras   = -1.0000 0.0000 0.0000
yras   = 0.0000 0.0000 -1.0000
zras   = -0.0000 1.0000 0.0000
cras   = -5.2736 9.0391 -27.2880
Read sample_brain_surface (10242 vertices 20480 triangles)
Reading /Users/alex/work/data/subjects/sample/mri/T1.mgz...[done]
Volume geometry information extracted from /Users/alex/work/data/subjects/sample/mri/T1.mgz
Did not ask to replace existing volume geometry

@larsoner
Copy link
Member

I won't get to it until next week at the earliest, but I'll have a look then. I assume #2193 is the higher priority?

@agramfort
Copy link
Member Author

agramfort commented Jun 10, 2015 via email

@agramfort
Copy link
Member Author

I played more...

Did not ask to replace existing volume geometry

error happens to with bash script but it does not make it fail. With Python we catch the error so python script fails...

just a status report...

agramfort
agramfort reviewed Jun 13, 2015
View reviewed changes
mne/bem.py
#
os.chdir(ws_dir)
if op.exists(T1_mgz):
# XXX : do this with python code
Copy link
Member Author

Choose a reason for hiding this comment

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

@Eric89GXL we'll take care of this later.

ready from end now.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for adding the reminder note

@dengemann
Copy link
Member

Are we in a rush here? Can you give me 24 hours or so to find a moment to try this with some of my data?

@agramfort
Copy link
Member Author

agramfort commented Jun 15, 2015 via email

@dengemann
Copy link
Member

No rush. I d like Lorenzo to move on that's all

Yeah, that's good. I'll have noted it and find a moment ASAP to check it out.

@larsoner
Copy link
Member

I finally have time to test this morning as well

larsoner
larsoner reviewed Jun 16, 2015
View reviewed changes
mne/utils.py
@@ -1025,7 +1032,8 @@ def set_memmap_min_size(memmap_min_size):
'MNE_CACHE_DIR',
'MNE_MEMMAP_MIN_SIZE',
'MNE_SKIP_TESTING_DATASET_TESTS',
'MNE_DATASETS_SPM_FACE_DATASETS_TESTS'
'MNE_DATASETS_SPM_FACE_DATASETS_TESTS',
'MNE_SKIP_SAMPLE_DATASET_TESTS'
Copy link
Member

Choose a reason for hiding this comment

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

we no longer need the MNE_SKIP_SAMPLE_DATASET_TESTS key

@larsoner larsoner modified the milestone: 0.10 Jun 16, 2015
@larsoner
Copy link
Member

@agramfort worked for me. I still wish we had a better test for the result, but I assume this will mostly be tested for quality by actual use in any case. +1 for merge after my last set of comments is addressed. And sorry for the delay in looking, I was away from my workstation that could actually handle the test.

@agramfort agramfort force-pushed the mne_watershed_bem branch from 4187257 to c2d34b0 Compare June 16, 2015 20:05
@agramfort
Copy link
Member Author

pushed with a rebase

@larsoner
Copy link
Member

Yes I am happy, thanks :) @dengemann feel free to merge when you're happy

@agramfort
Copy link
Member Author

@dengemann happy?

dengemann
dengemann reviewed Jun 20, 2015
View reviewed changes
mne/bem.py
os.makedirs(bem_dir)
if not op.exists(T1_dir) and not op.exists(T1_mgz):
raise RuntimeError('Could not find the MRI data')
if op.exists(ws_dir):
Copy link
Member

Choose a reason for hiding this comment

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

@agramfort here op.isdir would probably be more precise, the same way op.isfile would be more precise below ;)

@dengemann
Copy link
Member

@agramfort other than that running bems seems to work fine, also the overwrite is working.

dengemann
dengemann reviewed Jun 20, 2015
View reviewed changes
mne/utils.py
@@ -777,7 +784,7 @@ def run_subprocess(command, verbose=None, *args, **kwargs):
"that you use '$HOME' instead of '~'.")
warnings.warn(msg)

logger.info("Running subprocess: %s" % str(command))
logger.info("Running subprocess: %s" % ' '.join(command))
Copy link
Member

Choose a reason for hiding this comment

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

@agramfort can you share a screenshot or print of output generated here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

where?

Copy link
Member Author

Choose a reason for hiding this comment

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

screen shot 2015-06-20 at 11 00 58

Copy link
Member

Choose a reason for hiding this comment

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

perfect!

@dengemann
Copy link
Member

Travis is not happy.

@agramfort
Copy link
Member Author

the error seems unrelated. Test that fails is "Test creating fsaverage and scaling it"

@dengemann
Copy link
Member

Ok. Then +1 for merge.

agramfort added a commit that referenced this pull request Jun 20, 2015
@agramfort
Merge pull request #2194 from agramfort/mne_watershed_bem
b143c6d 
[MRG] Mne watershed bem
@agramfort agramfort merged commit b143c6d into mne-tools:master Jun 20, 2015
@agramfort
Copy link
Member Author

cool !

@lorenzo-desantis please open new PR with next feature.

@effigies effigies mentioned this pull request Aug 30, 2019
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.
5 participants
@agramfort @larsoner @dengemann @kingjr @lorenzo-desantis