Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Clean up irf/io.py and add load_cta_irf function #1909
This PR is a first step in cleaning up suggested in issue #1551.
It removes all CTAPerf and associated tables in irf/io.py which are connected to sensitivity estimation with prod 2 CTA IRFs. Those will be moved in gammapy-extra.
IS this OK @jjlk ?
@registerrier - Thanks!
Suggest to rename
load_cta_irfs - it's not just 1DC specific. Please put a properly formatted docstring or the Sphinx build will fail. Probably this will evolve to be a flexible loader that has some format discovery and object mapping and will just load all IRFs it finds and not error out if e.g. no BKG is given like for HESS is better, no? Otherwise we'll have to write one such helper function for each IRF. OK to leave as-is for now if you want, I'm just saying.
Why did you change from class to function? Isn't the next step again a method to slice out the response for a offset, and for that having a class to attach this method to is useful? Did you want to make this another function?
I don't think the old class from Julien was bad - it was just in the wrong place
gammapy.scripts and had a weird name (
CTAIRFLoader or something like that would be fine IMO) and all of the other not so useful classes that mainly did plotting were things I wanted to get rid of.
Feel free to merge as-is if you like. I don't care strongly about any of these points at the moment, this is clearly a bit step in the right direction and we won't get the API right now in a one-time effort anyways.
I decided to turn
Simulations should be performed on reduced datasets. So that the most natural approach to me is extract the reduced IRFs from a (set of) fake empty observations.
@registerrier - OK.
You could try and see if this "just works" with https://docs.gammapy.org/0.8/api/gammapy.data.ObservationCTA.html
We're not sure if we want to keep it as a second observation class, but the use case to build one from scratch and use it for simulation remains in whatever form this code takes, so
There is a Travis CI fail here: https://travis-ci.org/gammapy/gammapy/jobs/452289084#L4201
It is apparently due to the impossibility to find the gammapy dataset. Note that this test is done without gammapy-extra. I thought that using data stored in $GAMMAPY_DATA would work even without gammapy-extra. Am I wrong? @Bultako @cdeil
This is something that has to be changed and use
There's some discussion about removing
In the mean time, consider that Travis tests explicitly not requiring
We still have a ton of tests that access GAMMAPY_EXTRA:
@registerrier - Please do that for this PR.
@Bultako - I think getting rid of GAMMAPY_EXTRA for Gammapy testing isn't so simple. Probably adding
@registerrier - CI passed, this is mostly ready to go in.
I would suggest to remove the default
It might be convenient now, but really it's not doing us or the users a favour. In 2019 or 2020 that file and format will be old, and there users should use newer IRF files. So in Gammapy we would want to update the default filename. But then if a user relied on the default, their script breaks or they silently get different results. -> better to remove the default IMO.
You can still mention the default in the docstring or an