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

Fs surf metadata #458

Closed
wants to merge 6 commits into from
Closed

Fs surf metadata #458

wants to merge 6 commits into from

Conversation

agramfort
Copy link
Contributor

@agramfort agramfort commented Jun 7, 2016

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 95.825% when pulling 63fd6c9 on agramfort:fs_surf_metadata into 791b10f on nipy:master.

@larsoner
Copy link
Contributor

larsoner commented Jun 7, 2016

I see some debugging things in there, I assume you want someone to take over?

@effigies
Copy link
Member

effigies commented Jun 7, 2016

Ugh. Running up against Python 2.6's lack of OrderedDict. I guess we could just do a list of 2-tuples? If someone wants to access by name on 2.7+, they can just do OrderedDict(volume_info) themselves.

@jaeilepp
Copy link
Contributor

jaeilepp commented Jun 7, 2016

extra as well as the stuff at the end should be returned too, no? Otherwise you cannot recreate everything at write_geometry.

postlude = [b'\x00\x00\x00\x14']
for key, val in volume_info.items():
if key in ('voxelsize', 'xras', 'yras', 'zras', 'cras'):
val = '{:.3f} {:.3f} {:.3f}'.format(*val)
Copy link
Member

Choose a reason for hiding this comment

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

Realized these should be .4f.

@matthew-brett
Copy link
Member

We have our own copy of ordereddict for Python 2.6 in externals. But it's OK to drop Python 2.6 too.

import numpy as np
import getpass
import time
from collections import OrderedDict
Copy link
Member

Choose a reason for hiding this comment

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

as in from nibabel.externals import OrderedDict

@agramfort
Copy link
Contributor Author

comments addressed. Let's see if travis is happy.

@Eric89GXL what debug statements do you see?

@codecov-io
Copy link

codecov-io commented Jun 7, 2016

Current coverage is 93.74%

Merging #458 into master will decrease coverage by 0.03%

@@             master       #458   diff @@
==========================================
  Files           147        147          
  Lines         19181      19233    +52   
  Methods           0          0          
  Messages          0          0          
  Branches       2029       2042    +13   
==========================================
+ Hits          17988      18030    +42   
- Misses          796        803     +7   
- Partials        397        400     +3   

Powered by Codecov. Last updated by 791b10f...63fd6c9

if len(line) == 0:
continue
key, val = map(bytes.strip, line.split(b'=', 1))
print(key, val)
Copy link
Member

Choose a reason for hiding this comment

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

print statement

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 95.822% when pulling 995dc75 on agramfort:fs_surf_metadata into 791b10f on nipy:master.

@matthew-brett
Copy link
Member


Returns
-------
coords : numpy array
nvtx x 3 array of vertex (x, y, z) coordinates
faces : numpy array
nfaces x 3 array of defining mesh triangles
volume_info : dict-like
Copy link
Member

Choose a reason for hiding this comment

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

Say it's an ordered dict, given that we know that is the case?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.07% when pulling 89203bb on agramfort:fs_surf_metadata into 791b10f on nipy:master.

@agramfort
Copy link
Contributor Author

appveyor error seems unrelated

I have no idea why my last commit was necessary.

val = val.astype(np.int)
volume_info[key] = val
except ValueError:
raise ValueError("Error parsing volume info")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot of lines to be put in try. Which line is expected to fail? If it's only one (hopefully) then it's better to only have that line in the try clause.

If you're trying to catch any possible error on the other hand, then except Exception is more appropriate, and it would be good to do "Error parsing volume info: '%s'" % err

@larsoner
Copy link
Contributor

larsoner commented Jun 8, 2016

@agramfort while you're in there are you up for adding the quad stuff from MNE-Python?

@agramfort
Copy link
Contributor Author

agramfort commented Jun 8, 2016 via email

@jaeilepp jaeilepp mentioned this pull request Jun 10, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.076% when pulling 45055cd on agramfort:fs_surf_metadata into 791b10f on nipy:master.

@larsoner
Copy link
Contributor

Commit added to take care of my last comments (taking over for @agramfort). Ready for review/merge from my end.

@larsoner larsoner closed this Jun 10, 2016
@larsoner
Copy link
Contributor

Whoops, didn't mean to make that comment. Just meant to close. Closing in favor of #460

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.

BUG: write_geometry and read_geometry don't seem to be in the same coordinate system
7 participants