Move all data to standard data/ location#8413
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8413 +/- ##
=======================================
Coverage 86.77% 86.77%
=======================================
Files 387 387
Lines 58134 58134
Branches 1060 1060
=======================================
Hits 50446 50446
Misses 7073 7073
Partials 615 615
Continue to review full report at Codecov.
|
pllim
left a comment
There was a problem hiding this comment.
I have no objections, but only some comments and questions.
There is no need to touch astropy/extern? Won't those changes be overwritten anyway when external packages get updated via straight copy-and-paste?
If we discourage use of setup_package.py, should this sentiment be propagated to astropy-helpers and package-template too, as appropriate?
|
|
||
| def test_fk4_no_e_fk4(): | ||
| lines = get_pkg_data_contents('fk4_no_e_fk4.csv').split('\n') | ||
| lines = get_pkg_data_contents('data/fk4_no_e_fk4.csv').split('\n') |
There was a problem hiding this comment.
Since this PR is all about consistency, maybe use os.path.join here too? (Same comment applies to all subsequent repeats.)
There was a problem hiding this comment.
get_pkg_data_contents deals with this correctly on Windows, and in a lot of other places where we use this function we just use /, presumably for readability. So I'm inclined to leave as-is?
| from astropy.wcs.utils import pixel_to_skycoord | ||
|
|
||
| header = get_pkg_data_contents('../../wcs/tests/maps/1904-66_TAN.hdr', encoding='binary') | ||
| header = get_pkg_data_contents('../../wcs/tests/data/maps/1904-66_TAN.hdr', encoding='binary') |
There was a problem hiding this comment.
This makes me wonder if shared data files should be in an upper level data above both wcs and coordinates.
There was a problem hiding this comment.
Not a bad idea - but maybe worth doing in another PR since I'm not sure what the correct location should be.
| # Find all files in data/ sub-directories since this is a standard location | ||
| # for data files. We then need to adjust the paths to be relative to here | ||
| # (otherwise glob will be evaluated relative to setup.py) | ||
| data_files = glob.glob('**/data/**/*', recursive=True) |
There was a problem hiding this comment.
I guess this is a small price to pay for overall simplicity. Does this ever need to be modified if new data files or sub-packages are added? Or when they are removed?
There was a problem hiding this comment.
@pllim - this will look a lot simpler once I move the definition of the data to setup.cfg - it will look something like:
[options.package_data]
astropy = **/data/**/*
(plus other files). We don't need to change anything if new data folders are added, or old ones are removed, or files are added/removed inside them.
The only changes in extern are that I am moving the
Yes, but I first want to suggest this idea more formally on the mailing list so we can discuss it - the current PR still uses |
pllim
left a comment
There was a problem hiding this comment.
Tests passed and my comments/questions addressed. Might want to wait for others to approve too, just in case?
|
I removed a couple more |
bsipocz
left a comment
There was a problem hiding this comment.
This deals with an issue that bothered me for a long time! Thank you @astrofrog.
|
This is very nice. Just a cc to @drdavella since he is working quite a bit on the asdf schemas, and might like to have a heads-up (I was at first unsure whether that move was a good idea, but ended up thinking the consistency is well worth it). |
|
This looks really good overall. If it were totally up to me I might argue that the ASDF schemas should not be pushed into a |
eteq
left a comment
There was a problem hiding this comment.
LGTM too. I think this means we're all set and I'll merge this one.
In our coding guidelines, we say:
Packages can include data in a directory named data inside a subpackage source directory ...
Some sub-packages stick to the convention of using a
data/sub-directory, some don't. This PR cleans things up so that data files (excluding e.g. C/header files for now) are always in adata/sub-directory. This has the benefit that we can avoid having to usesetup_package.pyfor all different sub-packages just to defineget_package_data- instead, we add all the data indata/sub-directories once and for all in the rootsetup_package.py.Side note: the way we declare data files using
get_package_datainsetup_package.pyfiles is very astropy-idiosyncratic and over-complicates things especially for packages using the package template. The better way to declare package data is through thesetup.cfgfile, and this PR will make it easier to do so in future for the core package (thesetup.cfgfile supports glob syntax such as**/data/**/*). Anyway, this is for a future PR!