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 background estimation for phase-resolved spectra #1426

Merged
merged 3 commits into from Jul 11, 2018

Conversation

@msjacob
Copy link

@msjacob msjacob commented Jun 7, 2018

This is a first step for pulsar spectral analysis.

@cdeil cdeil self-assigned this Jun 7, 2018
@cdeil cdeil added the feature label Jun 7, 2018
@cdeil cdeil added this to the 0.8 milestone Jun 7, 2018
@cdeil cdeil added this to To do in gammapy.time via automation Jun 15, 2018
Copy link
Member

@cdeil cdeil left a comment

@msjacob - Thanks!
Apologies for the late response here!

I think this is a good addition. We can iterate on this a bit here, but basically I think we should get a working pulsar analysis in Gammapy and have a notebook docs example, and then once we see what we have we can still discuss if it can be structured in a better way.

The main thing that's missing here is a test case.
I wanted to suggest that you access something like

ds = DataStore.from_dir('$GAMMAPY_EXTRA/datasets/cta-1dc/index/gps/')

and use one or two obs from there for the test case.
But then I realised that there the PHASE column is missing.

I also see that you added this:
https://github.com/gammapy/gammapy-extra/tree/master/datasets/pulsar
but I think there the problem is that you don't easily get an ObservationList object, right?

There's a few options, ranging from preparing a test dataset that works, or changing your code. E.g. you work with ObservationList and DataStoreObservation objects in your methods, but then only access the EventList from there. So changing the API to take EventList objects instead could be possible. On the other hand, at some point for the spectral analysis you will need to correct the livetime for the on phase selection, so then it's not clear to me where that correction factor is best applied.

@msjacob - What do you think? Do you see a way to add a test?

The other suggestion I'd make is concerning the phase selection. Users will wonder how it works exactly and what is / isn't possible. Suggest to add some text or example to the docstring. For now, I think only a single interval is supported, and it can't wrap, i.e. be from 0.8 to 0.3. You don't do any wrapping of phases or support intervals that wrap below 0 or beyond 1. Can you state that in the docstring?
To have a more flexible phase selection API, I'm not sure how to do it. One idea could be to use lists of intervals, so that one can select e.g. an off phase from 0.1 to 0.2 AND from 0.5 to 0.7. Another idea could be to wrap PHASE to always 0 to 1 (or assume it's already wrapped like that), and then to also support wrapped phase selection, e.g. 0.7 to 0.2 would be like 0.7 to 1.0 AND 0.0 to 0.2. Another idea would be to introduce a PhaseSelection object that's just a few lines long for the default interval selection. But then users could write their own and pass it in to apply the phase selection. It would also have a "fraction" attribute that is then used for the livetime correction factor.

@msjacob - Any of those ideas / additions can come later or never. If you stick with the current implementation, please add two lines to the docstring to document what is done exactly and that there's this limitation that with the current scheme only simple interval phase selection isn't possible.

@@ -0,0 +1,75 @@
import numpy as np

This comment has been minimized.

@cdeil

cdeil Jun 19, 2018
Member

Add a copy of the two boilerplate lines that are at the top of any Python file in Gammapy (license and future imports).

@@ -0,0 +1,75 @@
import numpy as np
from gammapy.background.background_estimate import BackgroundEstimate

This comment has been minimized.

@cdeil

cdeil Jun 19, 2018
Member

Use relative imports within Gammapy:

from ..background.background_estimate import BackgroundEstimate
"""Background estimation with on and off phases.
This class is responsible for creating a
`~gammapy.background.BackgroundEstimate` by counting events in the on-phase-zone and off-phase-zone in an ON-region,

This comment has been minimized.

@cdeil

cdeil Jun 19, 2018
Member

Break this line.

This comment has been minimized.

@msjacob

msjacob Jul 11, 2018
Author

It’s done ! (All of the comments.)

@dcfidalgo
Copy link
Contributor

@dcfidalgo dcfidalgo commented Jul 10, 2018

@msjacob Nice!! I think the file should rather go to the folder gammapy/background instead of gammapy/spectrum

return BackgroundEstimate(
on_region=self.on_region,
on_events=on_events,
off_region=None,

This comment has been minimized.

@dcfidalgo

dcfidalgo Jul 10, 2018
Contributor

maybe rather than None it could hold the value self.on_region since the off events are retrieved from there

This comment has been minimized.

@msjacob

msjacob Jul 11, 2018
Author

I’d prefer to keep None because there is no off region and it seems clearer to me this way!

@cdeil cdeil assigned dcfidalgo and unassigned cdeil Jul 10, 2018
@dcfidalgo dcfidalgo force-pushed the msjacob:phase branch from a86dc75 to 035b2ac Jul 11, 2018
@dcfidalgo dcfidalgo dismissed cdeil’s stale review Jul 11, 2018

comments addressed

@dcfidalgo dcfidalgo force-pushed the msjacob:phase branch from fe5e50d to dc14952 Jul 11, 2018
Copy link
Contributor

@dcfidalgo dcfidalgo left a comment

Looks good now, merging

@dcfidalgo dcfidalgo merged commit dc705f0 into gammapy:master Jul 11, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.time automation moved this from To do to Done Jul 11, 2018
@cdeil cdeil changed the title New background estimation for phase-resolved spectra. Add background estimation for phase-resolved spectra Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.time
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants