Repository: spacetelescope/synphot_refactor

Issue:
have an option in download_data to download everything to the cache
Currently you have to specify a path for `download_data`.  That's fine for the use case illustrated in the docs, but if you don't use `download_data` but use some of the data files, the end result is a bunch of stuff in the cache. It would be nice if there was a way to instead tell `download_data` "download everything you need to the cache, because I'm not ready to work with it yet but am about to get on a flight (or otherwise be offline)".

I'll try to take a shot at this but putting it in now in case I don't find the time for a while.

Discussion:
pllim: Thanks! The failures are related to upstream refactoring, tsk tsk. I'll have to investigate that first before I come back to this. :grimacing: 

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 218ccd8..c0a4eac 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -8,6 +8,9 @@
   compatible with Python 3.6 or later and ``astropy`` 3.x or later. [#243]
 - Added support for ``RickerWavelet1D`` model that is the renamed version
   of ``MexicanHat1D`` model to be consistent with ``astropy`` 4.0. [#250]
+- Added option for ``synphot.utils.download_data()`` to download to the cache
+  instead of a specific location. Please note that new option is not fully
+  compatible with customization using ``synphot.cfg``. [#211]
 
 0.2.1 (2019-12-20)
 ==================
diff --git a/docs/index.rst b/docs/index.rst
index 9427f58..86901ae 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -82,10 +82,16 @@ via HTTP, create a local directory where you plan to store the data files
     >>> file_list = download_data('/my/local/dir/cdbs')  # doctest: +SKIP
 
 Then copy `synphot.cfg <https://github.com/spacetelescope/synphot_refactor/blob/master/synphot/synphot.cfg>`_
-to your ``$HOME/.astropy/config/`` directory, and replace every instance of
+to your ``$HOME/.astropy/config/`` directory, if it is not there already.
+Uncomment and replace every instance of
 ``/grp/hst/cdbs`` with ``/my/local/dir/cdbs`` so that ``synphot`` knows where to
 look for these files.
 
+On the contrary, if you wish to rely solely on Astropy caching mechanism,
+you can use ``download_data(None)``, but make sure that you do *not*
+modify your default ``$HOME/.astropy/config/synphot.cfg`` file. Otherwise,
+``synphot`` will try to use what is in ``synphot.cfg`` instead.
+
 .. note::
 
     **synphot** data files are a minimal subset of those required by
diff --git a/synphot/synphot.cfg b/synphot/synphot.cfg
index 4c2e7e3..c90f2bf 100644
--- a/synphot/synphot.cfg
+++ b/synphot/synphot.cfg
@@ -1,31 +1,31 @@
 [config]
 
 ## Default integrator
-default_integrator = 'trapezoid'
+#default_integrator = 'trapezoid'
 
 ## Vega
-vega_file = /grp/hst/cdbs/calspec/alpha_lyr_stis_009.fits
+#vega_file = /grp/hst/cdbs/calspec/alpha_lyr_stis_009.fits
 
 ## Reddening/extinction laws
-lmc30dor_file = /grp/hst/cdbs/extinction/lmc_30dorshell_001.fits
-lmcavg_file = /grp/hst/cdbs/extinction/lmc_diffuse_001.fits
-mwavg_file = /grp/hst/cdbs/extinction/milkyway_diffuse_001.fits
-mwdense_file = /grp/hst/cdbs/extinction/milkyway_dense_001.fits
-mwrv21_file = /grp/hst/cdbs/extinction/milkyway_rv21_001.fits
-mwrv40_file = /grp/hst/cdbs/extinction/milkyway_rv4_001.fits
-smcbar_file = /grp/hst/cdbs/extinction/smc_bar_001.fits
-xgal_file = /grp/hst/cdbs/extinction/xgal_starburst_001.fits
+#lmc30dor_file = /grp/hst/cdbs/extinction/lmc_30dorshell_001.fits
+#lmcavg_file = /grp/hst/cdbs/extinction/lmc_diffuse_001.fits
+#mwavg_file = /grp/hst/cdbs/extinction/milkyway_diffuse_001.fits
+#mwdense_file = /grp/hst/cdbs/extinction/milkyway_dense_001.fits
+#mwrv21_file = /grp/hst/cdbs/extinction/milkyway_rv21_001.fits
+#mwrv40_file = /grp/hst/cdbs/extinction/milkyway_rv4_001.fits
+#smcbar_file = /grp/hst/cdbs/extinction/smc_bar_001.fits
+#xgal_file = /grp/hst/cdbs/extinction/xgal_starburst_001.fits
 
 ## Common filter bandpass
-bessel_h_file = /grp/hst/cdbs/comp/nonhst/bessell_h_004_syn.fits
-bessel_j_file = /grp/hst/cdbs/comp/nonhst/bessell_j_003_syn.fits
-bessel_k_file = /grp/hst/cdbs/comp/nonhst/bessell_k_003_syn.fits
-cousins_i_file = /grp/hst/cdbs/comp/nonhst/cousins_i_004_syn.fits
-cousins_r_file = /grp/hst/cdbs/comp/nonhst/cousins_r_004_syn.fits
-johnson_b_file = /grp/hst/cdbs/comp/nonhst/johnson_b_004_syn.fits
-johnson_i_file = /grp/hst/cdbs/comp/nonhst/johnson_i_003_syn.fits
-johnson_j_file = /grp/hst/cdbs/comp/nonhst/johnson_j_003_syn.fits
-johnson_k_file = /grp/hst/cdbs/comp/nonhst/johnson_k_003_syn.fits
-johnson_r_file = /grp/hst/cdbs/comp/nonhst/johnson_r_003_syn.fits
-johnson_u_file = /grp/hst/cdbs/comp/nonhst/johnson_u_004_syn.fits
-johnson_v_file = /grp/hst/cdbs/comp/nonhst/johnson_v_004_syn.fits
+#bessel_h_file = /grp/hst/cdbs/comp/nonhst/bessell_h_004_syn.fits
+#bessel_j_file = /grp/hst/cdbs/comp/nonhst/bessell_j_003_syn.fits
+#bessel_k_file = /grp/hst/cdbs/comp/nonhst/bessell_k_003_syn.fits
+#cousins_i_file = /grp/hst/cdbs/comp/nonhst/cousins_i_004_syn.fits
+#cousins_r_file = /grp/hst/cdbs/comp/nonhst/cousins_r_004_syn.fits
+#johnson_b_file = /grp/hst/cdbs/comp/nonhst/johnson_b_004_syn.fits
+#johnson_i_file = /grp/hst/cdbs/comp/nonhst/johnson_i_003_syn.fits
+#johnson_j_file = /grp/hst/cdbs/comp/nonhst/johnson_j_003_syn.fits
+#johnson_k_file = /grp/hst/cdbs/comp/nonhst/johnson_k_003_syn.fits
+#johnson_r_file = /grp/hst/cdbs/comp/nonhst/johnson_r_003_syn.fits
+#johnson_u_file = /grp/hst/cdbs/comp/nonhst/johnson_u_004_syn.fits
+#johnson_v_file = /grp/hst/cdbs/comp/nonhst/johnson_v_004_syn.fits
diff --git a/synphot/utils.py b/synphot/utils.py
index 9bd00cd..2b53979 100644
--- a/synphot/utils.py
+++ b/synphot/utils.py
@@ -252,14 +252,21 @@ def merge_wavelengths(waveset1, waveset2, threshold=1e-12):
     return out_wavelengths
 
 
-def download_data(cdbs_root, verbose=True, dry_run=False):
-    """Download CDBS data files to given root directory.
+def download_data(path_root, verbose=True, dry_run=False):
+    """Download ``synphot`` data files to given root directory or
+    the ``astropy`` cache.
     Download is skipped if a data file already exists.
 
+    .. warning::
+
+        Downloading data to ``astropy`` cache only is not recommended
+        if you plan to provide a custom ``synphot.cfg``.
+
     Parameters
     ----------
-    cdbs_root : str
-        Root directory for CDBS data files.
+    path_root : str or `None`
+        Root directory for data files. If `None`, download to the ``astropy``
+        cache location instead of a specific directory.
 
     verbose : bool
         Print extra information to screen.
@@ -267,7 +274,8 @@ def download_data(cdbs_root, verbose=True, dry_run=False):
     dry_run : bool
         Go through the logic but skip the actual download.
         This would return a list of files that *would have been*
-        downloaded without network calls.
+        downloaded without network calls. The sub-directories
+        would still be created regardless.
         Use this option for debugging or testing.
 
     Raises
@@ -282,19 +290,20 @@ def download_data(cdbs_root, verbose=True, dry_run=False):
 
     """
     from .config import conf  # Avoid potential circular import
+    BASE_HOST = 'http://ssb.stsci.edu/cdbs/'
 
-    if not os.path.exists(cdbs_root):
-        os.makedirs(cdbs_root, exist_ok=True)
-        if verbose:  # pragma: no cover
-            print('Created {}'.format(cdbs_root))
-    elif not os.path.isdir(cdbs_root):
-        raise OSError('{} must be a directory'.format(cdbs_root))
+    if path_root is not None:
+        if not os.path.exists(path_root):
+            os.makedirs(path_root, exist_ok=True)
+            if verbose:  # pragma: no cover
+                print('Created {}'.format(path_root))
+        elif not os.path.isdir(path_root):
+            raise OSError('{} must be a directory'.format(path_root))
 
-    host = 'http://ssb.stsci.edu/cdbs/'
-    file_list = []
+        if not path_root.endswith(os.sep):
+            path_root += os.sep
 
-    if not cdbs_root.endswith(os.sep):
-        cdbs_root += os.sep
+    file_list = []
 
     # See https://github.com/astropy/astropy/issues/8524
     for cfgitem in conf.__class__.__dict__.values():
@@ -302,35 +311,44 @@ def download_data(cdbs_root, verbose=True, dry_run=False):
                 not cfgitem.name.endswith('file')):
             continue
 
-        url = cfgitem()
+        url = cfgitem.defaultvalue
 
-        if not url.startswith(host):
+        if not url.startswith(BASE_HOST):
             if verbose:  # pragma: no cover
                 print('{} is not from {}, skipping download'.format(
-                    url, host))
+                    url, BASE_HOST))
             continue
 
-        dst = url.replace(host, cdbs_root).replace('/', os.sep)
+        if path_root is not None:
+            dst = url.replace(BASE_HOST, path_root).replace('/', os.sep)
 
-        if os.path.exists(dst):
-            if verbose:  # pragma: no cover
-                print('{} already exists, skipping download'.format(dst))
-            continue
+            if os.path.exists(dst):
+                if verbose:  # pragma: no cover
+                    print('{} already exists, skipping download'.format(dst))
+                continue
 
-        # Create sub-directories, if needed.
-        subdirs = os.path.dirname(dst)
-        os.makedirs(subdirs, exist_ok=True)
+            # Create sub-directories, if needed.
+            subdirs = os.path.dirname(dst)
+            os.makedirs(subdirs, exist_ok=True)
 
         if not dry_run:  # pragma: no cover
             try:
-                src = download_file(url)
-                copyfile(src, dst)
+                src = download_file(url, cache=True)
+                if path_root is not None:
+                    copyfile(src, dst)
             except Exception as exc:
                 print('Download failed - {}'.format(str(exc)))
                 continue
 
-        file_list.append(dst)
+        if path_root is None:
+            if dry_run:
+                file_list.append(url)
+            else:  # pragma: no cover
+                file_list.append(src)
+        else:
+            file_list.append(dst)
+
         if verbose:  # pragma: no cover
-            print('{} downloaded to {}'.format(url, dst))
+            print('{} downloaded to {}'.format(url, file_list[-1]))
 
     return file_list


Test patch:
diff --git a/synphot/tests/test_utils.py b/synphot/tests/test_utils.py
index 6bc6145..b83b133 100644
--- a/synphot/tests/test_utils.py
+++ b/synphot/tests/test_utils.py
@@ -123,6 +123,9 @@ def test_download_bad_root(tmpdir):
     with pytest.raises(OSError):
         utils.download_data(cdbs_root, verbose=False)
 
+    with pytest.raises(FileNotFoundError):
+        utils.download_data('', verbose=False)
+
 
 def test_download_data(tmpdir):
     """Test data download helper in dry run mode."""
@@ -151,10 +154,23 @@ def test_download_data(tmpdir):
         f.write('\n')
 
     # Use case where user redefined data file to be non-STScI.
+    # While the given file will be used, default Vega is downloaded anyway.
     filename = [fname for fname in file_list_1
                 if fname.endswith('alpha_lyr_stis_009.fits')][0]
     os.remove(filename)
     with conf.set_temp('vega_file', '/custom/host/my_vega.fits'):
         file_list_2 = utils.download_data(
             cdbs_root, verbose=False, dry_run=True)
-    assert len(file_list_2) == 0
+    assert len(file_list_2) == 1 and file_list_2[0] == filename
+
+
+def test_download_data_cache_only():
+    """Test data download helper in dry run mode (cache only)."""
+
+    # Use case where user downloads all data into new dir.
+    # The other use cases in test_download_data() will depend on the native
+    # behavior of astropy caching and not tested here.
+    file_list_1 = utils.download_data(None, verbose=False, dry_run=True)
+    filename = file_list_1[0]
+    assert len(file_list_1) == 21
+    assert filename.startswith('http')