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

PIG 16 - Gammapy package structure #2274

Merged
merged 9 commits into from Aug 18, 2019

Conversation

@cdeil
Copy link
Member

commented Jul 5, 2019

We would like to discuss a few options to improve the Gammapy package structure for v0.14 (and v1.0 would be the same).

There are many options, but we expect that what will come out is that we do relatively small changes now, not a complete re-organisation. The discussion and document will still be valuable as a basis and reference for future developments.

Concretely, currently we propose to dissolve gammapy.background into other sub-packages.

We might add an overview / plan to clean up gammapy.utils here.
We don't want to discuss the big rename towards uniform class, function and parameter names in this PIG. That will come before v1.0, but a bit later, as another cleanup step after this PIG and several others that are open have been implemented.

@registerrier @adonath or anyone that wants to chime in at this early stage already (maybe @kosack or @cvaneldik with the HESS experience?) - please comment!

@cdeil cdeil added the pig label Jul 5, 2019
@cdeil cdeil added this to the 0.14 milestone Jul 5, 2019
@cdeil cdeil requested review from adonath and registerrier Jul 5, 2019
@cdeil cdeil self-assigned this Jul 5, 2019
@cdeil cdeil referenced this pull request Jul 5, 2019
@kosack

This comment has been minimized.

Copy link

commented Jul 8, 2019

We propose to remove gammapy.background, and to move reflected.py to gammapy.spectrum, and the other few things to gammapy.image or gammapy.cube.

I would like to understand a bit the reasoning for moving background computations to the spectrum module. Surely you can compute the background (even in 1D) for reasons other than computing a spectrum? It seems strange to mix those concepts.

Perhaps that implies a bad splitting of images/cubes/lightcurves/spectra: You could consider the approach of considering a spectrum as simply a cube with 1 spatial bin, 1 time bin, and multiple energy bins. In that case spectrum might be more appropriate in cube. Same with image: aren't images just cubes with 1 time and energy bin? It seems to be possible to merge all those into one structure. After all, they are all just "flux in a set of bins". That's perhaps a more drastic change, but it would clean up the code a lot I think, since computing a flux cube in time-energy-space is the same operation as a single lightcurve, or spectrum, or flux map, or combinations thereof (space-time, space-energy, time-energy)

@adonath

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

For maps we already gave up the distinction between the 2D (SkyImage in gammapy.image), 3D (SkyCube in gammapy.cube) and ND cases and moved to a uniform gammapy.maps. I think this was good change and we should continue in this direction. I agree with @kosack here, that if possible, we should try to give up the package separation in 1D, 2D, 3D analysis cases and rather structure it by classes / abstractions. So introduce gammapy.models with all the models (including base classes), introduce gammapy.datasets for all dataset classes (including base classes), and maybe even keep gammapy.background with all the 1D, 2D and 3D background estimation methods. I can see how this is a large proposed change, but if we don't do it for 1.0, when is a better time then?

@cdeil

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@kosack - Thanks for your comments!

I think for background-related code there's really the option to group it in gammapy.background or to have it in gammapy.spectrum for 1D background spectrum estimation methods that are usually regions-based like the reflected.py we have now, and for 3D maps based methods in gammapy.cube. Currently we have a mix, e.g. there's gammapy.cube.make_map_background_irf or gammapy.cube.models.BackgroundModel or gammapy.cube.MapMakerRing in gammapy.cube, and e.g. gammapy.background.RingBackgroundEstimator in gammapy.background. I don't see a clear distinction of what should go where, so naively I would think that dissolving gammapy.background and putting things more in one place for maps-based background estimation is a good idea. For reflected regions it's not easy to find a good home, it would have to go in gammapy.spectrum for now I guess, because gammapy.cube depends on gammapy.spectrum, and we should aim to avoid a circular dependency of spectrum and cube.

@kosack - Do you think keeping as-is or pulling all background-related code (namely the one I pointed out from gammapy.cube) into gammapy.background and to keep that is a good idea?


The suggestion to combine all data types of spectrum, images, cubes, lightcurves is probably a good one. I mention this in the first section in the Outlook. I just don't think we have any chance to get this done over the summer of 2019 and don't think we should change our plan and say "let's change everything and see how it goes, if we're not ready in the fall we'll do v1.0 sometime in 2020 or whenever ready". Doing this change for v2.0 aimed at fall 2020 sounds good to me. So this is mostly a comment for @adonath and @registerrier - think what realistically can be achieved, taking into account all the other work we still have to do.

@cdeil

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

Based on discussions at the Gammapy coding sprint, I've updated the proposal, and added a second Figure. We will discuss again today, and then I should be able to polish the text and finalise this proposal tomorrow.

cdeil added 2 commits Jul 18, 2019
@cdeil

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

@adonath @registerrier and anyone that has time - please do a final review of this PIG.
I would like to circulate it early next week.

Do I get a 🥇?

Copy link
Contributor

left a comment

Thanks @cdeil . This is OK for me. You might just add a typical example usage of the helper function you suggest. The changes described in models can make model handling more painful, it would be good to see how this can be tackled.

docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-016.rst Show resolved Hide resolved
docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Thanks @cdeil, I've left some final comments. Once those are addressed, the PIG is ready for circulation from my side...

docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-016.rst Show resolved Hide resolved
docs/development/pigs/pig-016.rst Outdated Show resolved Hide resolved
@cdeil

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

@registerrier @adonath - Thanks for the feedback.

Most of it is addressed in c88fd2b .
There are two unresolved conversations here concerning model factory and gammapy.image.
Please comment one more time; would be great to get this PIG out the stall today.

Copy link
Member

left a comment

Thanks @cdeil, I don‘t have any further comments!

@cdeil

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Final review period is over (see https://groups.google.com/d/msg/gammapy/DEbjzUFOe1o/_UsQsW4FDwAJ), didn't receive any further comments.

Changed PIG status and filled decision section, and did some minor RST formatting cleanup (zero content changes) in abe4a00 . Merging this now.

Thank you @adonath @registerrier @kosack and anyone who gave feedback on this PIG at the coding sprint in Erlangen!

I will file Github issues with the tasks to implement this and assign to the v0.14 milestone, to have the reminder and to discuss details if needed.

@cdeil cdeil merged commit 5acedb6 into gammapy:master Aug 18, 2019
8 of 9 checks passed
8 of 9 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
gammapy.gammapy Build #20190818.1 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
@cdeil

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Reminder issues to implement this PIG: #2319 #2320 #2321 #2322

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.