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

Finish clean-up of gammapy.scripts #1551

Closed
cdeil opened this issue Jul 20, 2018 · 8 comments
Closed

Finish clean-up of gammapy.scripts #1551

cdeil opened this issue Jul 20, 2018 · 8 comments

Comments

@cdeil
Copy link
Contributor

cdeil commented Jul 20, 2018

This issue is to continue the cleanup and discussion started in #1517 of gammapy.scripts.

What's there?

I removed the CTAObservationSimulation and ObservationParameters and Target class in #1517, and would also like to remove the remaining BgRateTable, Psf68Table, SensitivityTable, CTAIrf and CTAPerf.

There would be no loss in functionality really, but the users of those classes will have to adapt their scripts. From what I can see, the main remaining tasks here are:

I think the BgRateTable, Psf68Table and SensitivityTable should be removed without replacement. What they do is to wrap a Table with two columns, do linear interpolation and plotting. We can show with a docs example how to do that with a few lines, but I don't think we should maintain code for this in Gammapy.

Another major question is when to do this. IMO it can only be either next week (things removed / improved in v0.8) or the week after (things still available in v0.8, gone then in v0.9). I don't see how keeping this code longer is helpful for users, and it's a drag for us Gammapy developers. We need to focus our resources on building something great, quickly (see e.g. #1546), and we need "all hands on deck", both users for testing and devs not to be distracted by old stuff.

@jjlk - who is using this? is there anyone that wants or needs this in v0.8? Is using v0.7 an option? Or is updating your script to the new code and using v0.8, v0.9, ... an option?

I'm willing to compromise here and put it back for v0.8, of course our goal is to already now support CTA users and ongoing studies. But only if it's really needed / helpful. If you anyways want to keep doing these simulations / studies for the next months / years and want to keep using Gammapy, then you'll have to update your scripts. I'm available to help / pair code on this in the next 2 weeks to help with the transition.

@cdeil cdeil added this to the 0.8 milestone Jul 20, 2018
@cdeil cdeil self-assigned this Jul 20, 2018
@cdeil cdeil added this to To do in gammapy.irf via automation Jul 20, 2018
@cdeil cdeil assigned registerrier and unassigned cdeil Jul 27, 2018
@cdeil cdeil modified the milestones: 0.8, 0.9 Jul 27, 2018
@cdeil
Copy link
Contributor Author

cdeil commented Jul 27, 2018

Discussed in the Gammapy call today. @registerrier insisted to keep this for v0.8, and to first talk to @jjlk before cleaning this up, and promised to do the clean-up for v0.9. I would hope rather to do it directly after the v0.8 release, so that we can focus developments on obs sim / IRF to the right place for August / September already, not leave this "to the fall".

I'll put the CTASpectrumSimulation class back for now in a follow-up PR.

@cdeil cdeil modified the milestones: 0.9, 0.8 Aug 8, 2018
@registerrier
Copy link
Contributor

Since we need a place to share contributed code that does not belong to the core of gammapy, a working and practical solution proposed by @cdeil is to move these codes to gammapy-extra so that clean-up of gammapy can go-on.
Obviously some updates for these codes will be required once v0.8 will be released and we will work with their developers to make the update.

@cdeil
Copy link
Contributor Author

cdeil commented Aug 8, 2018

@registerrier - Thank you for agreeing to move the old code to the side for now, so that we can continue with the work on the new code in Gammapy.

I have put a copy of cta_irf.py and cta_utils.py in gammapy-extra here:
gammapy/gammapy-extra@b15ec9d

The example cta_simulation.ipynb is also there, it runs with those two files and otherwise Gammapy master at the moment, gives the same results.

This allows us to continue with the work on Gammapy, I will continue with this today.

@cdeil
Copy link
Contributor Author

cdeil commented Aug 20, 2018

I've removed cta_simulation.ipynb for now in
gammapy/gammapy-extra@586dcbf

@registerrier - Note that the class to load IRFs is still in https://github.com/gammapy/gammapy/blob/master/gammapy/irf/io.py , so that would be one place to start work on this, and then the main change needed is to put a background rate model in SpectrumSimulation as described in #876 and then that cta_simulation.ipynb notebook can come back.

@registerrier
Copy link
Contributor

The API of SensitivityEstimatorhas been modified in #1998 and a corrected CTA sensitivity notebook has been introduced in #2000 .
The proper introduction of a BackgroundModel for 1D analysis remains to be done according to the dataset PIG (#1986 ). The remaining connected issues are therefore #876 #809 and are postponed to v0.11.

@registerrier registerrier modified the milestones: 0.10, 0.11 Jan 25, 2019
@adonath adonath modified the milestones: 0.11, 0.12 Mar 11, 2019
@registerrier registerrier modified the milestones: 0.12, 0.13 May 28, 2019
@registerrier
Copy link
Contributor

This is close to being solved by the introduction SpectrumDatasetOnOff. Postponing to v0.13 for now.

@cdeil cdeil modified the milestones: 0.13, 0.14 Jul 18, 2019
@registerrier
Copy link
Contributor

The last remaining issue was implemented in #2411 with the usage of irf background for the simulation.
Closing now.

gammapy.irf automation moved this from To do to Done Sep 27, 2019
@cdeil
Copy link
Contributor Author

cdeil commented Sep 27, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.irf
  
Done
Development

No branches or pull requests

3 participants