Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve catalog classes and gammapy-extra data handling #364

Merged
merged 29 commits into from Oct 27, 2015

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Oct 7, 2015

This PR started as #341 by @JonathanDHarris, rebased on master. It looks OK to me (+270 lines, -2 lines). There's all commits from that PR here, except JonathanDHarris@9ace5e9 which was causing the trouble with the rebase. I had to do git rebase --skip and then it worked.

I started adding a web app gammapy-catalog-browse with the goal to browse Fermi and HESS catalogs, plot spectra, finder charts, etc.

Then I got sidetracked with how catalogs are fetched for tests: it's much better now, a requires_data and requires_dependency decorator was added in gammapy.utils.testing so that it's easy to declare what's needed in a given test, and files from the gammapy-extra repo are loaded via gammapy.datasets.gammapy_extra.filename(), and gammapy-extra is found via the GAMMAPY_EXTRA environment variable and is available on travis-ci so that now almost all tests run on travis-ci without needing --remote-data.

I'm merging this now and will continue working on the catalog classes and catalog browser in a new PR.

@cdeil cdeil added the feature label Oct 7, 2015
@cdeil cdeil self-assigned this Oct 7, 2015
@cdeil cdeil added this to the 0.4 milestone Oct 7, 2015
@cdeil cdeil mentioned this pull request Oct 7, 2015
@cdeil cdeil force-pushed the cdeil:fermi-cat-tool branch from 5bc1007 to dc51d11 Oct 23, 2015
@cdeil
Copy link
Member Author

@cdeil cdeil commented Oct 27, 2015

@kingj90 There's still an issue with gammapy-extra not being found in the travis-ci builds:
https://travis-ci.org/gammapy/gammapy/jobs/87645494#L659
And I want to tidy some things up concerning @remote_data ... ~ 1 more hour.

cdeil added 2 commits Oct 23, 2015
Remove bundled files from gammapy/datasets/data
They were moved to gammapy-extra/test_datasets/unbundled

Files from gammapy-extra are now accessed via GAMMAPY_EXTRA
and not treated as remote data

Introduce `requires_dependency` and requires_data` for tests.
@cdeil cdeil mentioned this pull request Oct 27, 2015
@cdeil cdeil force-pushed the cdeil:fermi-cat-tool branch from 5808cbd to d675fce Oct 27, 2015
@cdeil cdeil changed the title Add Fermi catalog tool Improve catalog classes and gammapy-extra data handling Oct 27, 2015
@cdeil
Copy link
Member Author

@cdeil cdeil commented Oct 27, 2015

With this PR all test tests that were marked @remote_data before now run on travis-ci:
https://travis-ci.org/gammapy/gammapy/jobs/87662698#L1215

Overall Gammapy test coverage improved from 44% to 49%:
https://coveralls.io/builds/3962398

cdeil added a commit that referenced this pull request Oct 27, 2015
Improve catalog classes and gammapy-extra data handling
@cdeil cdeil merged commit 7297ffa into gammapy:master Oct 27, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@cdeil cdeil mentioned this pull request Oct 27, 2015
@joleroi
Copy link
Contributor

@joleroi joleroi commented Oct 27, 2015

thanks a lot!

@cdeil
Copy link
Member Author

@cdeil cdeil commented Oct 27, 2015

@kingj90 – I broke some things (spectral analysis, cube background modeling) while restructuring the data handling code and marked tests as xfail in cases where I didn't want to spend the time to update the code. Sorry about that. E.g. re-activating this test and getting it working would be a good place to start for you:
https://github.com/gammapy/gammapy/blob/master/gammapy/spectrum/tests/test_spectrum_analysis.py#L8

@joleroi
Copy link
Contributor

@joleroi joleroi commented Oct 27, 2015

don't worry - this test were not implemented properly anyways. It's the next point on my agenda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants