search

nipy / PySurfer

Cortical neuroimaging visualization in Python
https://pysurfer.github.io/
BSD 3-Clause "New" or "Revised" License
243 stars 97 forks source link

make unittests usable without freesurfer's fsaverage #14

Closed yarikoptic closed 11 years ago

yarikoptic commented 12 years ago

Please refactor unittests so they operate on some very stripped down dataset (which you could ship along) instead of a bulky fsaverage from freesurfer. ATM:

$> python2.6 /usr/bin/nosetests -s -v surfer/tests
/usr/bin/nosetests:5: UserWarning: Module paste was already imported from None, but /usr/lib/python2.6/dist-packages is being added to sys.path
  from pkg_resources import load_entry_point
Failure: ValueError (Test suite relies on the definition of SUBJECTS_DIR) ... ERROR

======================================================================
ERROR: Failure: ValueError (Test suite relies on the definition of SUBJECTS_DIR)
----------------------------------------------------------------------
Traceback (most recent call last): 
  File "/usr/lib/python2.6/dist-packages/nose/loader.py", line 390, in loadTestsFromName
    addr.filename, addr.module)    
  File "/usr/lib/python2.6/dist-packages/nose/importer.py", line 39, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/usr/lib/python2.6/dist-packages/nose/importer.py", line 86, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/home/yoh/proj/nipy/nipy-suite/pysurfer/surfer/tests/test_io.py", line 10, in <module>
    raise ValueError('Test suite relies on the definition of SUBJECTS_DIR')
ValueError: Test suite relies on the definition of SUBJECTS_DIR
mwaskom commented 12 years ago

Here's the Freesurfer license, it's supposed to be "BSDish". http://surfer.nmr.mgh.harvard.edu/fswiki/FreeSurferSoftwareLicense

Do you see anything here that would be a problem?

yarikoptic commented 12 years ago

it is not about the license (yes -- it has been a year or so since FreeSurfer finally entered the FOSS world) it is the size

$> du -scm fsaverage 
245 fsaverage

so it is kinda "inefficient" to ask downloading 250MB of data to provide rudimentary testing, which I would like to perform for every distribution we build Debian package of pysurfer

mwaskom commented 12 years ago

Sure but I assume we still want to do it with fsaverage, just a restricted set of fsaverage files. So I wanted to double-check about the license.

larsoner commented 11 years ago

If it still matters, it looks like it would require 61MB to include a "minimal" fsaverage so that tests pass.

agramfort commented 11 years ago

@yarikoptic still interested in packaging? should the data be packaged separately? I am -1 for adding 60MB of data to main git repo... but if there is no other alternative....

yarikoptic commented 11 years ago

On Tue, 19 Mar 2013, Alexandre Gramfort wrote:

[1]@yarikoptic still interested in packaging?

I am confused:

$> acpolicy python-surfer
python-surfer:
  Installed: 0.3+git15-gae6cbb1-1.1
  Candidate: 0.3+git15-gae6cbb1-1.1
  Version table:
 *** 0.3+git15-gae6cbb1-1.1 0
        900 http://debian.lcs.mit.edu/debian/ wheezy/main amd64 Packages
        600 http://debian.lcs.mit.edu/debian/ sid/main amd64 Packages
        100 /var/lib/dpkg/status
     0.3+git15-gae6cbb1-1~nd70+1 0
        400 http://neuro.debian.net/debian/ wheezy/main amd64 Packages

pysurfer was packaged quite some time ago if you meant "Debian packaging" ;)

should the data be packaged separately?

Ideally -- yes...

I am -1 for adding 60MB of data to main git repo... but if there is not other alternative....

What is it? is it some generic dataset? it might then better have a life of its own ;)

Yaroslav O. Halchenko http://neuro.debian.net http://www.pymvpa.org http://www.fail2ban.org Senior Research Associate, Psychological and Brain Sciences Dept. Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755 Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419 WWW: http://www.linkedin.com/in/yarik

agramfort commented 11 years ago

I am confused:

$> acpolicy python-surfer
python-surfer:
Installed: 0.3+git15-gae6cbb1-1.1
Candidate: 0.3+git15-gae6cbb1-1.1
Version table:
*** 0.3+git15-gae6cbb1-1.1 0
900 http://debian.lcs.mit.edu/debian/ wheezy/main amd64 Packages
600 http://debian.lcs.mit.edu/debian/ sid/main amd64 Packages
100 /var/lib/dpkg/status
0.3+git15-gae6cbb1-1~nd70+1 0
400 http://neuro.debian.net/debian/ wheezy/main amd64 Packages

pysurfer was packaged quite some time ago if you meant "Debian packaging" ;)

oh boy ... did not know that ... :-/ good to know you package code with no test :)

should the data be packaged separately?

Ideally -- yes...

I am -1 for adding 60MB of data to main git repo... but if there is not other alternative....

What is it? is it some generic dataset? it might then better have a life of its own ;)

indeed...

yarikoptic commented 11 years ago

oh boy ... did not know that ... :-/ good to know you package code with no test :)

yikes -- I hope it was not intended to be offensive... Why do you think "yarikoptic opened this issue 11 months ago"?

So far I ended up running them manually, but so far majority of testing was simply via using it since coverage of viz is not there:

Name            Stmts   Miss  Cover   Missing
---------------------------------------------
surfer              3      0   100%
surfer.config       8      0   100%
surfer.io         214    106    50%   18-20, 41-62, 72, 103-105, 122-166, 197, 217, 259-260, 284-318, 362-407, 452, 469, 473
surfer.utils       63     54    14%   25-26, 43-49, 65-76, 98-130, 154-177
surfer.viz        736    668     9%   18-19, 24-28, 71-138, 158-167, 185-195, 221-239, 258-283, 323-413, 428-497, 519-572, 585-653, 683-720, 743-787, 807-815, 826-851, 857-859, 881-900, 918-937, 951-962, 983-998,
1023-1037, 1054-1104, 1121-1174, 1184-1195, 1207-1224, 1236, 1254-1273, 1292-1335, 1353-1364, 1368-1373, 1378-1424, 1428-1430, 1437-1506, 1510-1515, 1519-1524, 1566-1584, 1590-1593, 1599-1602, 1608-1611, 1617-1620
---------------------------------------------
TOTAL            1024    828    19%
----------------------------------------------------------------------
Ran 5 tests in 0.308s
agramfort commented 11 years ago

yikes -- I hope it was not intended to be offensive...

no just french humor :)

Why do you think "yarikoptic opened this issue 11 months ago"?

;)

So far I ended up running them manually, but so far majority of testing was simply via using it since coverage of viz is not there:

WIP. It will get better soon I guess...

larsoner commented 11 years ago

@yarikoptic this software is indeed meant to be used in with a standard, separate dataset that is distributed with software called Freesurfer. For people who have the dataset (in other words, nearly anyone who would use this PySurfer python package), I've recently implemented some tests that are likely to increase our coverage quite substantially. In principle it would be possible to test for the presence of this standard dataset and, if it's not present, skip the tests. This would artificially make it seem as though our test coverage was poor, but it would also allow tests to pass on your end (assuming you don't have Freesurfer installed). Would that be a workable solution?

yarikoptic commented 11 years ago

@Eric89GXL sorry about the delay with reply. I know about pysurfer and its intimate relation with Freesurfer. All my "Russian humor" was about:

  1. ideally, do not ship (large) data which comes from another package and altogether might (or will) have its own life
  2. requiring 60MB of data for unit testing of few kB of code sounds really like over kill to me, especially if tests are really rudimentary and more of smoke tests (btw -- great to see test_viz.py coming along)
  3. IO -- as much as possible should be moved to nibabel and all the io testing should be left to it:
Date: Mon, 7 May 2012 22:28:40 +0200
From: Alexandre Gramfort <alexandre.gramfort@inria.fr>
To: nipy-devel@neuroimaging.scipy.org
Subject: Re: [Nipy-devel] install Pysurfer
X-CRM114-Status: UNSURE (5.8227) This message is 'unsure'; please train it!

| ah -- so the next step is to RF pysurfer to use nibabel's
| functionality instead of an internal copy?

yes now that nibabel is released.
  1. if not -- you could easily come up with a minimalistic condensed data set for the purpose of unittests (e.g. nibabel carries 680B .mgz for its tests). In PyMVPA for some similar use cases to test functionality (not IO) surfaces simply get generated (e.g. a simple sphere etc)

That would allow me to enable at least some tests at package build time in the official Debian (if offscreen rendering is not possible... haven't tried yet suggested way by Gael yet (mlab.options.offscreen = False)), where freesurfer's dataset(s) are N/A atm.

hope this clarifies my position ;-)

agramfort commented 11 years ago

+1 for using nibabel for IO.

btw I moved in nibabel.freesurfer my code from surfer.io so it's a pure duplicate.

larsoner commented 11 years ago

@mwaskom @agramfort do we have a separate, downloadable minimal fsaverage anywhere? If not, I think we should package one and put it up somewhere.

For example, I have a collaborator who would like to view some data I sent, and PySurfer would be an ideal way to do it. However, they don't have FreeSurfer. While I'm making them a minimal fsaverage to use, I figure we might as well put it up somewhere for people to download if they want. Is there somewhere on MGH's servers or the PySurfer Github site we could stash it?

While I'm at it, I'll try running the test suite without FreeSurfer installed (i.e., on Windows 7). If tests pass, I'll close this. If they don't, I'll tweak them to make them pass without FreeSurfer data, and submit a PR (and then close this).

mwaskom commented 11 years ago

Yeah that sounds good; this is something ostensibly on the Freesurfer folks' radar, so we could also try to bug them about it.

On Thu, Sep 26, 2013 at 9:41 PM, Eric89GXL notifications@github.com wrote:

@mwaskom https://github.com/mwaskom @agramforthttps://github.com/agramfortdo we have a separate, downloadable minimal fsaverage anywhere? If not, I think we should package one and put it up somewhere.

For example, I have a collaborator who would like to view some data I sent, and PySurfer would be an ideal way to do it. However, they don't have FreeSurfer. While I'm making them a minimal fsaverage to use, I figure we might as well put it up somewhere for people to download if they want. Is there somewhere on MGH's servers or the PySurfer Github site we could stash it?

While I'm at it, I'll try running the test suite without FreeSurferinstalled (i.e., on Windows 7). If tests pass, I'll close this. If they don't, I'll tweak them to make them pass without FreeSurfer data, and submit a PR (and then close this).

— Reply to this email directly or view it on GitHubhttps://github.com/nipy/PySurfer/issues/14#issuecomment-25222519 .

larsoner commented 11 years ago

Closing this for PR regarding tests.