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

Use GAMMAPY_DATA in Gammapy codebase #1822

Merged
merged 1 commit into from Sep 21, 2018

Conversation

Projects
None yet
2 participants
@Bultako
Member

Bultako commented Sep 21, 2018

This PR replaces $GAMMAPY_EXTRA by $GAMMAPY_DATA hardcoded in the codebase of Gammapy, which prevents reproducibility of tutorials and catalog access for users not having env vars like $GAMMAPY_EXTRA or $GAMMA_CAT.

We can see $GAMMAPY_DATA has a data-repo stuck to Gammapy that is defined in http://gammapy.org/download/data/gammapy-data-index.json The codebase of Gammapy needs $GAMMAPY_DATA, only a few exceptional cases (mostly related with gammacat) may still be fixed.

  • gammapy download datasets downloads the whole $GAMMAPY_DATA repo.
  • gammapy download notebooks downloads does not download the repo.
  • gammapy download tutorials downloads a big fraction of this repo, almost all the datasets of $GAMMAPY_DATA are used in the tutorials.

I have left $GAMMAPY_EXTRA in the test_*.py scripts, this env var is used by developers to access datasets that will not be provided to users.

I'm afraid developers should have $GAMMAPY_EXTRA and $GAMMAPY_DATA pointing to the same path ? export GAMMAPY_DATA=$GAMMAPY_EXTRA/datasets

Local regression test pass, and three notebooks remain broken for those not having $GAMMAPY_FERMI_LAT_DATA or $GAMMA_CATdeclared:

  • fermilat needs $GAMMAPY_FERMI_LAT_DATA
  • sed_fitting_gammacat_fermi needs $GAMMA_CAT
  • spectrum_pipe needs $GAMMA_CAT

@Bultako Bultako requested a review from cdeil Sep 21, 2018

@Bultako Bultako self-assigned this Sep 21, 2018

@Bultako Bultako added the cleanup label Sep 21, 2018

@cdeil cdeil added this to the 0.8 milestone Sep 21, 2018

@cdeil cdeil changed the title from Replace and fix env vars in Gammapy codebase to Use GAMMAPY_DATA in Gammapy codebase Sep 21, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 21, 2018

I'm afraid developers should have $GAMMAPY_EXTRA and $GAMMAPY_DATA pointing to the same path ?

I don't have it pointing to the same path. I just have them separately.

Since not all files from GAMMAPY_DATA come from gammapy-extra (maybe that's still the case, but it won't remain like this), I think the proper dev setup is to just have them separately.

We can consider completely raplacing the use of GAMMAPY_EXTRA by GAMMAPY_DATA, also for test files, but that might not be a great solution (exposing test datasets to users a bit), and anyways isn't high priority.

I think for devs it's acceptable to have an extra 5 min of setup and to clone and set up GAMMAPY_EXTRA as well. Of course, we need to improve the Gammapy developer docs and explain the steps / setup clearly, but then I think it'll be OK.

@cdeil

@Bultako - Thanks!

Let me know if I should do anything (e.g. edit notebooks).

For gamma-cat, it's particularly complicated because it's a "live catalog", the version at https://gamma-cat.readthedocs.io/output/gammacat.fits.gz can change any day if a new source is discovered and if someone has time to make a data entry there.

But for testing and tutorials, we probably want a fixed, pre-downloaded version.

That makes it especially complicated.

But let's leave that discussion for another day, and for now everywhere just use the fixed version in gammapy-extra.

@Bultako

This comment has been minimized.

Member

Bultako commented Sep 21, 2018

Ok, thanks.
It's fine for me, I still have some clean up to finish and then I slow down a bit in the next week.. :)

@Bultako Bultako force-pushed the Bultako:access-catalogs branch from b7449c8 to cc6db65 Sep 21, 2018

@Bultako Bultako merged commit 18fafdd into gammapy:master Sep 21, 2018

0 of 4 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Running
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Bultako Bultako deleted the Bultako:access-catalogs branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment