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

Remove cta_utils and CTASpectrumObservation #1517

Merged
merged 2 commits into from Jul 16, 2018

Conversation

3 participants
@cdeil
Member

cdeil commented Jul 16, 2018

@joleroi - Look, I found another Target class to delete, following your lead in #1490.

This PR removes gammapy.scripts.cta_utils, which had a CTASpectrumObservation, which from what I can tell does almost exactly what gammapy.spectrum.SpectrumSimulation does. There even was a TODO comment that the two should be merged.

I would suggest to merge this now and continue removing duplicated / non-essential things.

But then we still should make the existing examples that people in CTA use work again, i.e. migrate this notebook to use gammapy.utils.SpectrumSimulation:
http://docs.gammapy.org/dev/notebooks/cta_simulation.html

@cdeil cdeil added the cleanup label Jul 16, 2018

@cdeil cdeil added this to the 0.8 milestone Jul 16, 2018

@cdeil cdeil added this to To do in Spectrum analysis (1D) via automation Jul 16, 2018

@cdeil cdeil changed the title from Rm cta sim to Remove cta_utils and CTASpectrumObservation Jul 16, 2018

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 16, 2018

Member

@jjlk - FYI.

Member

cdeil commented Jul 16, 2018

@jjlk - FYI.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 16, 2018

Member

I'll just go ahead. We can always bring back stuff if people complain and really some features get lost in the cleanup.

Member

cdeil commented Jul 16, 2018

I'll just go ahead. We can always bring back stuff if people complain and really some features get lost in the cleanup.

@cdeil cdeil merged commit bbc63db into gammapy:master Jul 16, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Spectrum analysis (1D) automation moved this from To do to Done Jul 16, 2018

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Jul 17, 2018

Contributor

Agreed

But then we still should make the existing examples that people in CTA use work again, i.e. migrate this notebook to use gammapy.utils.SpectrumSimulation:
http://docs.gammapy.org/dev/notebooks/cta_simulation.html

I put it on my agenda

Contributor

joleroi commented Jul 17, 2018

Agreed

But then we still should make the existing examples that people in CTA use work again, i.e. migrate this notebook to use gammapy.utils.SpectrumSimulation:
http://docs.gammapy.org/dev/notebooks/cta_simulation.html

I put it on my agenda

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Jul 17, 2018

Contributor

Hi @cdeil , @joleroi ,
Those utility classes are currently used in the analyses of several ongoing CTA consortium papers (EBL, LIV, AGN, GRB, Pevatron). I think we have to wait a bit before removing everything (I'm not the only user of those scripts).

(CC to @registerrier , @bkhelifi , @santiagopita , @jlenain , @ctrichard , @cboisson, and +)

Contributor

jjlk commented Jul 17, 2018

Hi @cdeil , @joleroi ,
Those utility classes are currently used in the analyses of several ongoing CTA consortium papers (EBL, LIV, AGN, GRB, Pevatron). I think we have to wait a bit before removing everything (I'm not the only user of those scripts).

(CC to @registerrier , @bkhelifi , @santiagopita , @jlenain , @ctrichard , @cboisson, and +)

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 17, 2018

Member

@jjlk and all - we're not removing everything. The strategy is to never remove something, unless there is an equivalent or better way to do the same thing.

In this case, I think gammapy.scripts.cta_utils.CTASpectrumObservation was basically a duplicate of gammapy.spectrum.SpectrumSimulation, except that the API was slightly different how the IRF object is passed in. So updating should be a matter of adapting the import and a few lines, changing your script to gammapy.spectrum.SpectrumSimulation. We're more than happy to help with this, feel free to ask anytime or complain about specific things on Slack or mailing list of Github issues. In some cases also using Gammapy v0.7 and keeping things as-is might be an option instead of updating to Gammapy v0.8.

Just to explain the reason for the cleanup: we have 37,000 lines of code in Gammapy, and about half of that is duplication of functionality. We have to move to a better, clean package to be able to succeed with Gammapy in general, and for the CTA ST selection in the fall specifically. So we're trying to balance short-term pain for the few (10s) of current users with the long-term benefit for the coming years for 100s or 1000s of users. This removal of e.g. gammapy.scripts.cta_utils.CTASpectrumObservation has to come, we can't have multiple classes that do the same thing long-term, we don't have the manpower to maintain, and it's also a disservice to users. If really needed, we can add it back, but please try to change to SpectrumSimulation first and let us know if that doesn't work for you.

All - please join the Gammapy call this Friday 10 am (https://github.com/gammapy/gammapy-meetings/tree/master/2018-07-20) or let me know if a separate short call would be helpful.

Member

cdeil commented Jul 17, 2018

@jjlk and all - we're not removing everything. The strategy is to never remove something, unless there is an equivalent or better way to do the same thing.

In this case, I think gammapy.scripts.cta_utils.CTASpectrumObservation was basically a duplicate of gammapy.spectrum.SpectrumSimulation, except that the API was slightly different how the IRF object is passed in. So updating should be a matter of adapting the import and a few lines, changing your script to gammapy.spectrum.SpectrumSimulation. We're more than happy to help with this, feel free to ask anytime or complain about specific things on Slack or mailing list of Github issues. In some cases also using Gammapy v0.7 and keeping things as-is might be an option instead of updating to Gammapy v0.8.

Just to explain the reason for the cleanup: we have 37,000 lines of code in Gammapy, and about half of that is duplication of functionality. We have to move to a better, clean package to be able to succeed with Gammapy in general, and for the CTA ST selection in the fall specifically. So we're trying to balance short-term pain for the few (10s) of current users with the long-term benefit for the coming years for 100s or 1000s of users. This removal of e.g. gammapy.scripts.cta_utils.CTASpectrumObservation has to come, we can't have multiple classes that do the same thing long-term, we don't have the manpower to maintain, and it's also a disservice to users. If really needed, we can add it back, but please try to change to SpectrumSimulation first and let us know if that doesn't work for you.

All - please join the Gammapy call this Friday 10 am (https://github.com/gammapy/gammapy-meetings/tree/master/2018-07-20) or let me know if a separate short call would be helpful.

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Jul 17, 2018

Contributor

I underdstand your points. But please keep in mind that the main reason we introduced CTASpectrumObservation was to handle the FITS point-like IRFs, that we converted from ROOT files provided by the ASWG group, with the CTAPerf class (also written in the ~gammapy.scripts). And please consider also that the tools have been tested and used since more than one year.

I'll try to join the call next Friday ++

Contributor

jjlk commented Jul 17, 2018

I underdstand your points. But please keep in mind that the main reason we introduced CTASpectrumObservation was to handle the FITS point-like IRFs, that we converted from ROOT files provided by the ASWG group, with the CTAPerf class (also written in the ~gammapy.scripts). And please consider also that the tools have been tested and used since more than one year.

I'll try to join the call next Friday ++

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 17, 2018

Member

@jjlk - I understand.

But we still have to do this clean-up, either now before the v0.8 release or in a few weeks after the v0.8 release, or at the very latest before Gammapy v1.0 this fall.

Note that this cleanup of CTASpectrumObservation and CTAPerf in gammapy.scripts is just a small part of all the work that is going on in Gammapy. We've also introduced gammapy.maps and are very far along with the rewrite of all map-based analysis. That means every user of SkyImage and SkyCube will have to adapt their scripts in the coming weeks. The same holds for the other parts of Gammapy, e.g. gammapy.irf or gammapy.data or gammapy.spectrum.

Let's work together to make this transition. I think on the Gammapy side, the changes to SpectrumSimulation needed to support your analyses are small (I just saw #876, moving this to the v0.8 milestone now), and also for you the time investment to update your scripts for your papers should be small, if you choose to update to Gammapy v0.8 for them. And we're happy to help.

I'll try to join the call next Friday ++

Great!

Member

cdeil commented Jul 17, 2018

@jjlk - I understand.

But we still have to do this clean-up, either now before the v0.8 release or in a few weeks after the v0.8 release, or at the very latest before Gammapy v1.0 this fall.

Note that this cleanup of CTASpectrumObservation and CTAPerf in gammapy.scripts is just a small part of all the work that is going on in Gammapy. We've also introduced gammapy.maps and are very far along with the rewrite of all map-based analysis. That means every user of SkyImage and SkyCube will have to adapt their scripts in the coming weeks. The same holds for the other parts of Gammapy, e.g. gammapy.irf or gammapy.data or gammapy.spectrum.

Let's work together to make this transition. I think on the Gammapy side, the changes to SpectrumSimulation needed to support your analyses are small (I just saw #876, moving this to the v0.8 milestone now), and also for you the time investment to update your scripts for your papers should be small, if you choose to update to Gammapy v0.8 for them. And we're happy to help.

I'll try to join the call next Friday ++

Great!

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