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 Target class #546

Merged
merged 16 commits into from Jun 5, 2016

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Jun 3, 2016

This PR add ~gammapy.data.Target and restructures the spectrum analysis module to use it.

  • add gammapy.data.Target and gammapy.data.TargetSummary as minimal Analysis class using the Target class
  • add gammapy.background.BackgroundEstimate, container class for background estimates for a Target
  • Move gammapy.spectrum.CountsSpectrum to gammapy.spectrum.core and refactor (using NDData)
  • add a new class OnCountsSpectrum (as mirror of a PHA file)
  • Move gammapy.spectrum.SpectrumObservation to gammapy.spectrum.core and simply (only container without any functionality, meta is stored in the OnCountsSpectrum, as in OGIP)
  • Refactore gammapy.spectrum.SpectrumExtraction to take only a Target instance as input
  • Rewrite gammapy.utils.scripts to use new infrastructure

The gammapy.spectrum.spectrum_fit module is not updated, and does not work after this PR. Another PR will fix this soon.

@joleroi joleroi added this to the 0.5 milestone Jun 3, 2016

@cdeil cdeil self-assigned this Jun 3, 2016

@@ -6,7 +6,7 @@
from ..reflected import find_reflected_regions
from ...image import ExclusionMask
from ..circle import SkyCircleRegion
from regions.shapes import CircleSkyRegion

This comment has been minimized.

@cdeil

cdeil Jun 3, 2016

Member

This will fail on travis-ci.
Do you have time to make a separate PR now to bundle gammapy.extern.regions and then we merge that one first?

This comment has been minimized.

@joleroi

joleroi Jun 3, 2016

Contributor

Yes, that was my plan.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 3, 2016

This PR does a few big changes. Can you please put a better description at the top (e.g. a bullet list)? One thing I noticed is that SpectrumObservation is removed.

class BackgroundEstimate(object):
"""Cointainer class for background estimate

This comment has been minimized.

@cdeil

cdeil Jun 3, 2016

Member

Typo "Cointainer" -> "Container"

class Target(object):
"""Observation Target.
This class represents an observation target

This comment has been minimized.

@cdeil

cdeil Jun 3, 2016

Member

Remove this line. (or de-dent it, but I don't think it adds useful extra info)

This is meant to hold the result from a region based background estimation
for one observation.
Parameters:

This comment has been minimized.

@cdeil

cdeil Jun 3, 2016

Member

Remove the colon here. Otherwise this will give a warning and make the Sphinx build fail on travis-ci.

Background estimation method
"""
def __init__(self, off_region, off_events, alpha, tag='default'):
self.tag = tag

This comment has been minimized.

@cdeil

cdeil Jun 3, 2016

Member

Move this line to the end of the function. (Code is easier to read and maintain if things are always in the same order, i.e. as in the docstring and function signature)

@joleroi joleroi changed the title from Introduce Target class to WIP: Introduce Target class Jun 3, 2016

@joleroi

This comment has been minimized.

Contributor

joleroi commented Jun 3, 2016

Once tests pass I will go through the docstring again and make the more usefull/add examples. So please only comment if you have general remarks on the API/class structure at the moment.

@joleroi joleroi force-pushed the joleroi:target branch from c651deb to 33b93e9 Jun 4, 2016

joleroi added some commits Jun 4, 2016

@joleroi joleroi changed the title from WIP: Introduce Target class to Introduce Target class Jun 4, 2016

joleroi added some commits Jun 4, 2016

fix
fix

@joleroi joleroi merged commit cbea9d0 into gammapy:master Jun 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joleroi joleroi deleted the joleroi:target branch Jun 5, 2016

----------
off_events : `~gammapy.data.EventList`
Background events
off_region : `~gammapy.extern.regions.SkyRegion`

This comment has been minimized.

@cdeil

cdeil Jun 5, 2016

Member

Put off_region before off_events in the docstring.
(Always use the same order in the function signature and docstring.)

"""
off_region = find_reflected_regions(on_region, pointing, exclusion)
off_events = events.select_circular_region(off_region)
alpha = len(off_region)

This comment has been minimized.

@cdeil

cdeil Jun 5, 2016

Member

This is very confusing to IACT people, where all papers use alpha so that excess = n_on - alpha * n_off.
I think you're using alpha as what is sometimes called areafactor = 1 / alpha.

I'm OK with using either areafactor or alpha as a data member, but please use it in the common way.

This comment has been minimized.

@joleroi

joleroi Jun 5, 2016

Contributor

I think you're using alpha as what is sometimes called areafactor = 1 / alpha.

No, this is just an error 😄

This comment has been minimized.

@cdeil

cdeil Jun 5, 2016

Member

What about alpha = ring_area_factor(on_radius, inner_radius, outer_radius) above?
Should this be 1 / as well?

This comment has been minimized.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 5, 2016

Thanks!

Locally I'm seeing this fail?
https://gist.github.com/cdeil/f01065eda3493dea7ba20c9e62d2c66e
I think you're aware and there's a follow-up PR coming?

@joleroi

This comment has been minimized.

Contributor

joleroi commented Jun 5, 2016

Locally I'm seeing this fail?

You have to update gammapy-extra

@joleroi joleroi referenced this pull request Jun 6, 2016

Merged

Fixes to gammapy.spectrum #553

@cdeil cdeil changed the title from Introduce Target class to Add Target class Jun 9, 2016

@cdeil cdeil removed the infrastructure label Jun 9, 2016

@joleroi joleroi referenced this pull request Jun 24, 2016

Merged

Finish change to use gammapy.extern.regions #558

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