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 1D spectrum analysis tool based on gammapy.hspec #359

Merged
merged 15 commits into from Sep 30, 2015

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Sep 23, 2015

This PR introduces the GammapySpectrumObservation and GammapySpectrumAnalysis classes that are the basis for the command line tool gammapy-spectrum-pipe. This tool performs a basic 1D spectral analysis using a ring in the FoV to estimated the background contamination

Missing functionality

  • tests are not possible because data handling is based on ~gammapy.obs.DataStore for which also no tests are implemented
  • Reflected regions
@cdeil

This comment has been minimized.

Member

cdeil commented Sep 23, 2015

I think your code will simplify quite a bit and be more re-usable from other scripts / algorithms if you introduce separate functions (and maybe even a class) to represent one observation and do the processing per observation.

Maybe something like this?

class GammapySpectrumObservation:
    event list data member
    count data member and method to compute it
    effective area data member and method to compute it
    energy resolution data member and method to compute it

and then your analysis class only does the batch observation processing / book-keeping:

class GammaSpectrumAnalysis:
    def process_observations
        for obs in self.observations:
            obs.process()

For the fitting from Sherpa I don't know yet how it works ... I haven't looked at how it's done by hspec.
But at least for computing PHA, ARF, RMFs I'd recommend the approach mentioned above.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Sep 24, 2015

I absolutely agree, thank you!

@joleroi

This comment has been minimized.

Contributor

joleroi commented Sep 28, 2015

updated, please have a look

from ..data import CountsSpectrum, EventList
from ..spectrum import EnergyBounds, Energy
from astropy.coordinates import Angle, SkyCoord
import sherpa.astro.ui as sau

This comment has been minimized.

@cdeil

cdeil Sep 28, 2015

Member

The Sherpa imports have to be moved inside the functions / methods, because it's an optional dependency:
https://travis-ci.org/gammapy/gammapy/jobs/82537346#L973

def test_spectrum_pipe(tmpdir):
#Change to remote file in the end
configfile = "~/Software/gammapy/gammapy/scripts/spectrum_pipe_example.yaml"

This comment has been minimized.

@cdeil

cdeil Sep 28, 2015

Member

This test currently fails:
https://travis-ci.org/gammapy/gammapy/jobs/82537347#L1483

Either mark it @pytest.mark.xfail and add a TODO: implement test later.
Or actually implement the test using this one run for now:
https://github.com/gammapy/gammapy-extra/tree/master/test_datasets/irf/hess/pa

model:
type: PWL
threshold_low: 0.1

This comment has been minimized.

@cdeil

cdeil Sep 28, 2015

Member

Is it possible / better to have the unit on the same line, i.e. have the string "0.1 TeV" here that then gets passed to the Quantity constructor?
If this doesn't work out of the box or you don't think it's better that way, OK to leave as is.

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 28, 2015

This looks good!

Can you make --help print something useful?
One option would be to replace this old docs page (https://gammapy.readthedocs.org/en/latest/tutorials/gammapy-pfspec/index.html) with an example showing how to use this script, and link to the online URL from the --help text?

@cdeil cdeil added the feature label Sep 29, 2015

@cdeil cdeil added this to the 0.4 milestone Sep 29, 2015

@cdeil cdeil assigned cdeil and joleroi and unassigned cdeil Sep 29, 2015

def run_fit(self):
log.info("Starting HSPEC")
if (self.model == 'PWL'):

This comment has been minimized.

@cdeil

cdeil Sep 29, 2015

Member

Can you please use "PL" instead of "PWL"?
I have some thoughts how to generalise this for different models (split it out in model classes and a factory function), but that can come later.

p1.ref = 1e9
p1.ampl = 6e-19
else:
log.error('Desired Model is not defined')

This comment has been minimized.

@cdeil

cdeil Sep 29, 2015

Member

It probably doesn't make sense to continue here, because you'll just raise a more cryptic error below when p1 isn't defined. So you should raise a ValueError here instead of emitting a log.error message.

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 29, 2015

Please run autopep8 on the new gammapy/scripts/spectrum_pipe.py before committing the file.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Sep 29, 2015

Hi I applied most of your comments, I would like to do the rest in the separate PR, like

  • edit --help output and write example page
  • implement ring background
  • Write alpha keyword correctly

Of course I have to fix the build error here :(

@joleroi joleroi force-pushed the joleroi:spectrum_pipe branch from 2edf6bf to 512c85f Sep 30, 2015

@joleroi

This comment has been minimized.

Contributor

joleroi commented Sep 30, 2015

There is one (allowed) test failure. If there are no objections I am going to merge this, and continue in another PR

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 30, 2015

👍

Can you please change the title of the PR to be a bit more descriptive (e.g. "Add 1D spectrum analysis tool") and add a changelog entry yourself?

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 30, 2015

It would also be nice if you edit the issue description at the top to contain a short description of what this PR does ... makes it easy to understand what's what in the future.

@joleroi joleroi changed the title from WIP: Spectrum pipe to Add 1D spectrum analysis tool Sep 30, 2015

@joleroi joleroi changed the title from Add 1D spectrum analysis tool to Add 1D spectrum analysis tool based on gammapy.hspec Sep 30, 2015

joleroi pushed a commit that referenced this pull request Sep 30, 2015

Merge pull request #359 from kingj90/spectrum_pipe
Add 1D spectrum analysis tool based on gammapy.hspec

@joleroi joleroi merged commit 41d6c8e into gammapy:master Sep 30, 2015

1 check passed

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

@joleroi joleroi deleted the joleroi:spectrum_pipe branch Sep 30, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 30, 2015

🆒

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