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

Use GAMMAPY_DATA in Gammapy codebase #1822

Merged
merged 1 commit into from
Sep 21, 2018
Merged

Conversation

Bultako
Copy link
Member

@Bultako 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 September 21, 2018 16:22
@Bultako Bultako self-assigned this Sep 21, 2018
@cdeil cdeil added this to the 0.8 milestone Sep 21, 2018
@cdeil cdeil changed the title Replace and fix env vars in Gammapy codebase Use GAMMAPY_DATA in Gammapy codebase Sep 21, 2018
@cdeil
Copy link
Contributor

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
cdeil previously approved these changes Sep 21, 2018
Copy link
Contributor

@cdeil cdeil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

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 merged commit 18fafdd into gammapy:master Sep 21, 2018
@Bultako Bultako deleted the access-catalogs branch September 21, 2018 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants