add option for download_data to use the astropy cache #211
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thank you! Overall, the logic seems fine.
However, the caching mechanism only works if user opts out of using the synphot.cfg altogether? I don't see how something like this will work:
- Vega -- Download and cache
- Extinction files -- Download and save to somewhere
Also, the cache location is not easy to find, so how do you envision the situation where synphot user wants to invalidate the cache (force a re-download) for whatever reason?
Would you have time to add user-facing doc for this PR as well?
This comment has been minimized.
This comment has been minimized.
pllim
force-pushed
the
auto-download-cache
branch
from
149fda1
to
c8f505e
Compare
add clarifying note about cfg Add changelog entry
pllim
force-pushed
the
auto-download-cache
branch
from
c8f505e
to
a9dfba1
Compare
|
I'll push follow-up commits and wrap this up. I think testing is possible if we stick to dry run mode and maybe some Mock (not sure but will try). |
synphot/utils.py
Outdated
| @@ -252,14 +252,20 @@ 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=None, verbose=True, dry_run=False): | |||
There was a problem hiding this comment.
I am reverting the code to make path_root optional. It changes the default behavior and might confuse users.
|
I cannot find an easy way for "cache only" option to work with |
|
|
||
| # See https://github.com/astropy/astropy/issues/8524 | ||
| for cfgitem in conf.__class__.__dict__.values(): | ||
| if (not isinstance(cfgitem, ConfigItem) or | ||
| not cfgitem.name.endswith('file')): | ||
| continue | ||
|
|
||
| url = cfgitem() | ||
| url = cfgitem.defaultvalue |
There was a problem hiding this comment.
This line fixes #188 but also changes the behavior reflected in the diff for test_download_data. I think the behavior change is a small price to pay for this fix.
This PR closes #208 by making it possible to use the astropy download cache.
One caveat: There are no unit tests. I started trying to make one but then realized it's not clear to me that we can really test this properly without accidentally messing with the users' "true" cache. The related tests in astropy test the cache machinery by downloading a "fake" file and then clearing just that file, but here we can't do it because we actually want the correct file. So that's awkward. I can concoct some sort of complicated scheme like what we used to have in astropy where we fake the cache location, run the test, and then un-fake it, but that seems like it might be overkill for this. @pllim do you agree and/or have a simpler idea for how I might test this? Or alternatively I can add a quick test that would have the side-effect of potentially deleting the user's actual copy of the data (which is normally considered bad behavior for a test...)
(Note I did test it in the sense of actually running it myself... )
EDIT: Also fix #188