Repository: rmarkello/abagen

Issue:
Improve data fetcher path handling
## The issue(s)

If a user does not have a local copy of all the Allen Institute human microarray data then, when calling `abagen.get_expression_data`, the data are automatically fetched. Currently, this fetching procedure will create a directory in the user's current directory called `allenbrain` and all the data are downloaded and unzipped in there. Users can optionally provide a path to the `data_dir` parameter, but this requires that the specified path be to a directory _inside of which_ is a directory labelled `allenbrain` with all the necessary data. This can be confusing as many people think that `data_dir` should be the `allenbrain` directory itself.

Furthermore, most users might not want to have to constantly point to the directory where they store it (or deal with constantly downloading duplicate copies of it in their current working directory). It might be nice to think about using a $HOME directory (a la `nilearn`, `sklearn`, etc.) to store the downloaded data and attempt to fetch from, if no `data_dir` path is provided.

## Proposed solutions

To address the first issue we could modify the checking of the `data_dir` parameter to accept EITHER the current scenario (a path to a directory inside of which is a directory labelled `allenbrain` with all the necessary data) OR a path to a directory inside of which is all the Allen Brain Atlas data (i.e., it does not need to be labelled `allenbrain`). So, providing either `data_dir=/home/ross/Desktop` or `data_dir=/home/ross/Desktop/allenbrain` in the following example should work:

```python
>>> import abagen
>>> atlas = abagen.fetch_desikan_killiany()

# allenbrain directory exists on Desktop
>>> data_dir = '/home/ross/Desktop'
>>> abagen.get_expression_data(atlas.image, data_dir=data_dir)

# data_dir points to allenbrain directory itself
>>> data_dir = '/home/ross/Desktop/allenbrain'
>>> abagen.get_expression_data(atlas.image, data_dir=data_dir)
```

assuming that `/home/ross/Desktop/allenbrain` was organized as:

```
allenbrain/
├── normalized_microarray_donor9861/
├── normalized_microarray_donor10021/
...
└── normalized_microarray_donor15697/
```

To address the second issue we could consider creating the `allenbrain` directory in the user's $HOME directory by default (instead of in the current directory). If it doesn't already exist there, we can do a quick check in the current working directory (and the `data_dir` path, if provided) and, if nothing else exists, fetch the data to the $HOME directory.

Discussion:

Patch:
diff --git a/.travis.yml b/.travis.yml
index 83bcf6c..f292493 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,81 +1,74 @@
 language: python
+cache: pip
 sudo: false
-dist: xenial
 notifications:
   email: change
 
 python:
   - 3.5
   - 3.6
-  - 3.7
 
 env:
-  matrix:
-    - CHECK_TYPE=linting
-    - CHECK_TYPE=docdoctest
-    - CHECK_TYPE=test
-  global:
-    - INSTALL_TYPE=setup
+  - LINTING=1
+  - DOCTEST=1
 
 matrix:
   include:
-  - python: 3.6
+  - python: 3.5
+    sudo: required
     env:
-      - INSTALL_TYPE=sdist
-      - CHECK_TYPE=test
+      - PARQUET=1
   - python: 3.6
+    sudo: required
     env:
-      - INSTALL_TYPE=wheel
-      - CHECK_TYPE=test
-  - python: 3.6
+      - PARQUET=1
+      - COVERAGE=1
+  - python: 3.7
+    dist: xenial
+    sudo: required
+  - python: 3.7
+    dist: xenial
     sudo: required
     env:
-      - INSTALL_PARQUET=true
-      - CHECK_TYPE=test
+      - DOCTEST=1
 
 before_install:
   - python -m pip install --upgrade pip
-  - if [ "${CHECK_TYPE}" == "linting" ]; then
+  - if [ "${LINTING}" == "1" ]; then
       pip install flake8;
     fi
-  - if [ "${CHECK_TYPE}" == "test" ]; then
-      pip install "pytest>=3.6" pytest-cov coverage coveralls codecov;
+  - if [ "${COVERAGE}" == "1" ]; then
+      pip install coverage coveralls codecov;
     fi
-  - if [ ! -z "${INSTALL_PARQUET}"]; then
+  - if [ "${PARQUET}" == "1" ]; then
       sudo apt-get install libsnappy-dev;
-      pip install fastparquet python-snappy;
     fi
 
 install:
-  - |
-    if [ "${INSTALL_TYPE}" == "setup" ]; then
-      python setup.py install;
-    elif [ "${INSTALL_TYPE}" == "sdist" ]; then
-      python setup.py sdist;
-      pip install dist/*.tar.gz;
-    elif [ "${INSTALL_TYPE}" == "wheel" ]; then
-      python setup.py bdist_wheel;
-      pip install dist/*.whl;
+  - if [ "${PARQUET}" == "1" ]; then
+      pip install -e .[io,tests];
+    elif [ "${DOCTEST}" == "1" ]; then
+      pip install -e .[doc];
     else
-      false;
+      pip install -e .[tests];
     fi
 
 script:
   - |
-    if [ "${CHECK_TYPE}" == "linting" ]; then
+    if [ "${LINTING}" == "1" ]; then
       flake8 abagen;
-    elif [ "${CHECK_TYPE}" == "docdoctest" ]; then
+    elif [ "${DOCTEST}" == "1" ]; then
       cd docs;
-      pip install -r ./requirements.txt;
       make html;
       make doctest;
-    elif [ "${CHECK_TYPE}" == "test" ]; then
-      travis_wait python -m pytest --cov-report term-missing --cov=abagen --doctest-modules abagen;
     else
-      false;
+      if [ "${COVERAGE}" == "1" ]; then
+        TEST_ARGS="--cov-report term-missing --cov=abagen";
+      fi
+      travis_wait py.test ${TEST_ARGS} abagen;
     fi
 
 after_success:
-  - if [ "${CHECK_TYPE}" == "test" ]; then
+  - if [ "${COVERAGE}" == "1" ]; then
       coveralls; codecov;
     fi
diff --git a/abagen/datasets.py b/abagen/datasets.py
index cf40747..1de9729 100644
--- a/abagen/datasets.py
+++ b/abagen/datasets.py
@@ -10,7 +10,7 @@ working directory, but will likely be modified to download into a more
 import os
 from pkg_resources import resource_filename
 from nibabel.volumeutils import Recoder
-from nilearn.datasets.utils import _fetch_files, _get_dataset_dir
+from nilearn.datasets.utils import _fetch_files
 import pandas as pd
 from sklearn.utils import Bunch
 from abagen import io
@@ -29,6 +29,103 @@ VALID_DONORS = sorted(WELL_KNOWN_IDS.value_set('subj') |
                       WELL_KNOWN_IDS.value_set('uid'))
 
 
+def _get_dataset_dir(dataset_name, data_dir=None, verbose=1):
+    """
+    Gets path to `dataset_name`
+
+    Parameters
+    ----------
+    dataset_name : str
+        The name of the dataset in question
+    data_dir : str, optional
+        Path to use as data directory. If not specified, will check for
+        environmental variables 'ABAGEN_DATA'; if that is not set, will use
+        '~/abagen-data' instead. Default: None
+    verbose : int, optional
+        Verbosity level (0 means no message). Default: 1
+
+    Returns
+    -------
+    data_dir : str
+        Path to use as data directory
+
+    References
+    ----------
+    Function lightly modified from `nilearn <https://github.com/nilearn/
+    nilearn>`_, which is licensed under the BSD, referenced here:
+
+    Copyright (c) 2007 - 2015 The nilearn developers.
+    All rights reserved.
+
+    Redistribution and use in source and binary forms, with or without
+    modification, are permitted provided that the following conditions are met:
+
+    a. Redistributions of source code must retain the above copyright notice,
+        this list of conditions and the following disclaimer.
+    b. Redistributions in binary form must reproduce the above copyright
+        notice, this list of conditions and the following disclaimer in the
+        documentation and/or other materials provided with the distribution.
+    c. Neither the name of the nilearn developers nor the names of
+        its contributors may be used to endorse or promote products
+        derived from this software without specific prior written
+        permission.
+
+    THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+    AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+    IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+    ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR
+    ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+    DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+    SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+    CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+    LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+    OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+    DAMAGE.
+    """
+
+    paths = [
+            os.environ.get('ABAGEN_DATA'),
+            os.path.join('~', 'abagen-data'),
+            os.getcwd()
+    ]
+    if data_dir is not None:
+        paths = [data_dir, os.path.dirname(data_dir)] + paths
+    paths = [os.path.expanduser(d) for d in paths if d is not None]
+
+    if verbose > 2:
+        print('Dataset search paths: {}'.format(paths))
+
+    for path in paths:
+        if not os.path.basename(path) == dataset_name:
+            path = os.path.join(path, dataset_name)
+        if os.path.islink(path):
+            link = os.readlink(path)
+            if os.path.isabs(link):
+                path = link
+            path = os.path.join(os.path.dirname(path), link)
+        if os.path.exists(path) and os.path.isdir(path):
+            if verbose > 1:
+                print('\nDataset found in {}\n'.format(path))
+            return path
+
+    errors = []
+    for path in paths:
+        if not os.path.basename(path) == dataset_name:
+            path = os.path.join(path, dataset_name)
+        if not os.path.exists(path):
+            try:
+                os.makedirs(path)
+                if verbose > 0:
+                    print('\nDataset created in {}\n'.format(path))
+                return path
+            except (NotADirectoryError, PermissionError) as exc:
+                err_msg = getattr(exc, 'strerror', str(exc))
+                errors.append('\n -{} ({})'.format(path, err_msg))
+
+    raise OSError('Tried to store dataset {} in the following directories, '
+                  'but: ' + ''.join(errors))
+
+
 def fetch_microarray(data_dir=None, donors=['9861'], resume=True, verbose=1,
                      convert=True):
     """
@@ -68,8 +165,6 @@ def fetch_microarray(data_dir=None, donors=['9861'], resume=True, verbose=1,
     url = "https://human.brain-map.org/api/v2/well_known_file_download/{}"
 
     dataset_name = 'allenbrain'
-    if data_dir is None:
-        data_dir = os.getcwd()
     data_dir = _get_dataset_dir(dataset_name, data_dir=data_dir,
                                 verbose=verbose)
 
diff --git a/abagen/mouse/gene.py b/abagen/mouse/gene.py
index 148f7c3..8c5b5fb 100644
--- a/abagen/mouse/gene.py
+++ b/abagen/mouse/gene.py
@@ -100,7 +100,7 @@ def get_gene_info(id=None, acronym=None, name=None, attributes=None,
         attributes = _GENE_ATTRIBUTES
     elif isinstance(attributes, str):
         attributes = [attributes]
-    attributes = [a for a in attributes if a not in provided]
+    attributes = list(set.difference(set(attributes), set([provided])))
 
     for attr in attributes:
         if attr not in _GENE_ATTRIBUTES:
diff --git a/abagen/mouse/mouse.py b/abagen/mouse/mouse.py
index 3884ed7..d7455d5 100644
--- a/abagen/mouse/mouse.py
+++ b/abagen/mouse/mouse.py
@@ -230,10 +230,10 @@ def get_unionization_from_gene(id=None, acronym=None, name=None,
     --------
     >>> from abagen import mouse
     >>> mouse.get_unionization_from_gene(acronym='Pdyn', structures=[22, 31])
-                          expression_density
-    gene_id structure_id
-    18376   22                      0.024840
-            31                      0.017199
+                  expression_density
+    structure_id
+    22                      0.024840
+    31                      0.017199
 
     >>> mouse.get_unionization_from_gene(acronym=['Ace', 'Cd99'], structures=[22, 31])
                           expression_density
diff --git a/abagen/mouse/structure.py b/abagen/mouse/structure.py
index 8a5b160..22e009a 100644
--- a/abagen/mouse/structure.py
+++ b/abagen/mouse/structure.py
@@ -77,11 +77,10 @@ def get_structure_info(id=None, acronym=None, name=None, attributes=None,
     Get the full names of structures 22 and 1018:
     >>> from abagen import mouse
     >>> mouse.get_structure_info(id=[22, 1018], attributes=['acronym', 'name'])
-         acronym                                  name
+                                          name acronym
     id
-    22      PTLp  Posterior parietal association areas
-    1018    AUDv                 Ventral auditory area
-
+    22    Posterior parietal association areas    PTLp
+    1018                 Ventral auditory area    AUDv
     """
 
     criteria = [
@@ -96,7 +95,7 @@ def get_structure_info(id=None, acronym=None, name=None, attributes=None,
         attributes = _STRUCTURE_ATTRIBUTES
     elif isinstance(attributes, str):
         attributes = [attributes]
-    attributes = [a for a in attributes if a not in provided]
+    attributes = list(set.difference(set(attributes), set([provided])))
 
     for attr in attributes:
         if attr not in _STRUCTURE_ATTRIBUTES:
diff --git a/setup.cfg b/setup.cfg
deleted file mode 100644
index 11c209a..0000000
--- a/setup.cfg
+++ /dev/null
@@ -1,2 +0,0 @@
-[tool:pytest]
-doctest_optionflags = NORMALIZE_WHITESPACE


Test patch:
diff --git a/abagen/tests/test_datasets.py b/abagen/tests/test_datasets.py
index 391e518..699d420 100644
--- a/abagen/tests/test_datasets.py
+++ b/abagen/tests/test_datasets.py
@@ -1,6 +1,9 @@
-import numpy as np
+import os
+import shutil
+
 import pandas as pd
 import pytest
+
 from abagen import datasets
 
 KEYS = [
@@ -8,6 +11,37 @@ KEYS = [
 ]
 
 
+def test_get_dataset_dir(testdir):
+    os.environ.pop('ABAGEN_DATA', None)
+
+    # check that data dir defaults to $HOME/abagen-data assuming no env var
+    expected_base = os.path.expanduser('~/abagen-data')
+    data_dir = datasets._get_dataset_dir('test', verbose=0)
+    assert data_dir == os.path.join(expected_base, 'test')
+    assert os.path.isdir(data_dir) and os.path.exists(data_dir)
+    shutil.rmtree(data_dir)
+
+    # if env var is set, we should default to that
+    expected_base = os.path.join(testdir, 'test-abagen-data')
+    os.environ['ABAGEN_DATA'] = expected_base
+    data_dir = datasets._get_dataset_dir('test', verbose=0)
+    assert data_dir == os.path.join(expected_base, 'test')
+    assert os.path.isdir(data_dir) and os.path.exists(data_dir)
+    shutil.rmtree(data_dir)
+
+    # test explicitly setting the data_dir
+    expected_dir = testdir
+    data_dir = datasets._get_dataset_dir('test', data_dir=expected_dir,
+                                         verbose=0)
+    assert data_dir == os.path.join(expected_dir, 'test')
+    assert os.path.isdir(data_dir) and os.path.exists(data_dir)
+    # test that providing the returned data_dir gets us the same thing
+    data_dir2 = datasets._get_dataset_dir('test', data_dir=data_dir,
+                                          verbose=0)
+    assert data_dir == data_dir2
+    shutil.rmtree(data_dir)
+
+
 def test_fetch_datasets(testdir):
     # check downloading for a subset of donors
     files = datasets.fetch_microarray(data_dir=str(testdir),
@@ -28,7 +62,7 @@ def test_fetch_alleninf_coords():
     coords = datasets._fetch_alleninf_coords()
     assert isinstance(coords, pd.DataFrame)
     assert coords.index.name == 'well_id'
-    assert np.all(coords.columns == ['mni_x', 'mni_y', 'mni_z'])
+    assert list(coords.columns) == ['mni_x', 'mni_y', 'mni_z']
     assert coords.shape == (3702, 3)