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

Add region info to CountsSpectrum and adapt tutorials #2493

Merged
merged 9 commits into from Oct 29, 2019

Conversation

@adonath
Copy link
Member

adonath commented Oct 28, 2019

This PR add region information to the CountsSpectrum object and adapts the tutorials to use this information for plotting. It concludes the following changes:

  • Add CountsSpectrum.region attribute
  • Implement compound_region_to_list() and list_to_compound_region() helper functions
  • Implement CountsSpectrum.plot_region() method
  • Implement plot_spectrum_datasets_off_regions helper function
  • Adapt tutorials to the new SpectrumDatasetMaker reduction scheme
@adonath adonath self-assigned this Oct 28, 2019
@adonath adonath added the cleanup label Oct 28, 2019
@adonath adonath added this to the 0.15 milestone Oct 28, 2019
@adonath adonath force-pushed the adonath:add_counts_spectrum_region branch from 85c0394 to 30f891b Oct 28, 2019
@adonath adonath changed the title Add region info to `CountSpectrum` and adapt tutorials Add region info to `CountsSpectrum` and adapt tutorials Oct 28, 2019
@adonath adonath force-pushed the adonath:add_counts_spectrum_region branch from 30f891b to 9acc798 Oct 28, 2019
@adonath adonath force-pushed the adonath:add_counts_spectrum_region branch from 9acc798 to c0d9449 Oct 28, 2019
@adonath adonath added this to To do in gammapy.spectrum via automation Oct 29, 2019
@adonath adonath requested a review from registerrier Oct 29, 2019
Copy link
Contributor

registerrier left a comment

Thanks @adonath . This looks good.
The only thing that might be missing is the OFF regions merging for the stacking. The meaning of the regions in that case is not very clear, so I am not sure whether we need it or not.

@adonath

This comment has been minimized.

Copy link
Member Author

adonath commented Oct 29, 2019

Thanks @registerrier! I agree we have to think about how to handle the region information when stacking. I guess up to a certain point it makes sense to keep it. On the other hand if you stack O(1e3) observations and each of the observations has >10 off regions, you end up with a lot of regions info, that you might not want to keep. The same question goes for the serialisation I guess. I think ideally the region info would be serialised along with the data, but for a stacked observation with O(1e3) individual regions it just becomes complex...

@adonath adonath merged commit 6f32173 into gammapy:master Oct 29, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 4 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191028.11 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
gammapy.spectrum automation moved this from To do to Done Oct 29, 2019
@adonath adonath changed the title Add region info to `CountsSpectrum` and adapt tutorials Add region info to CountsSpectrum and adapt tutorials Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.