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

Implement SpectrumDatasetMaker #2464

Merged
merged 7 commits into from Oct 16, 2019

Conversation

@adonath
Copy link
Member

adonath commented Oct 16, 2019

This PR implements a first version of a SpectrumDatasetMaker. It follows the design of the MapDatasetMaker closely. The plan is that SpectrumExraction and the ReflectedRegionBackgroundEstimator will be replaced by the following pattern, step by step:

spec_maker = SpectrumDatasetMaker(...)
bkg_maker = ReflectedRegionsBackgroundMaker(...)
datasets = []
for obs in observations:
    dataset = spec_maker.run(obs)
    dataset = bkg_maker.run(dataset, obs)
    datasets.append(dataset)

I will implement the ReflectedRegionsBackgroundMaker in a subsequent PR and start adapting tutorials and tests, so that we can finally remove the SpectrumExtraction class.

Copy link
Contributor

registerrier left a comment

Thanks @adonath . This looks good to me, except the fact that binsz and radius are free parameters.
We could have some helper function like the ones you've introduced here to compute the effective radius of a region, its solid angle. This would be useful too to build the Geom to use for the MapDatasetMaker in case one uses it for an extended source 1D analysis.

e_true=None,
containment_correction=True,
binsz="0.01 deg",
width="0.5 deg",

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

I think it is unsafe to leave width as an argument with a default value.
Ideally we should compute it from the region itself. Or even better the region should be able to compute it.

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

Technically it would be better if binsz is also defined w.r.t. the region size since it will impact strongly the estimate of the solid angle.

This comment has been minimized.

Copy link
@adonath

adonath Oct 16, 2019

Author Member

I decided to only support circular regions in make_background and hard coded to the analytical solution for the solid angle. This way I could also remove the binsz and width parameters, because for the event selection it does not matter.

This seemed to me the best compromise for now. In case people want to use reflected regions with different region shapes, they are not interested in the background spectrum anyway and can just disable it in SpectrumDatasetMaker.run().

"""Reference geometry to project region"""
coordsys = frame_to_coordsys(self.region.center.frame.name)
return WcsGeom.create(
skydir=self.region.center,

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

Note that this will fail for SkyPolygon (but so does the current implementation of the ReflectedRegionsBackgroundEstimator).

if self.containment_correction:
if not isinstance(self.region, CircleSkyRegion):
raise TypeError(
"Containment correction only support for circular regions."

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

only supported for

This comment has been minimized.

Copy link
@adonath

adonath Oct 16, 2019

Author Member

Fixed

)
psf = observation.psf

if isinstance(psf, EnergyDependentMultiGaussPSF):

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

why not use if not isinstance(psf, PSF3D): this will fail if to_psf3d is not implemented a be more general in case wa have to add other PSF irfs...

This comment has been minimized.

Copy link
@adonath

adonath Oct 16, 2019

Author Member

Actually I realised this call is not necessary at all, because all our current psf irf classes support the .to_energy_dependent_table_psf() method. I just removed it...

observation: `DataStoreObservation`
Observation to reduce.
selection : list
List of str, selecting which maps to make.

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

selecting which spectral products or spectral quantities to make

This comment has been minimized.

Copy link
@adonath

adonath Oct 16, 2019

Author Member

Fixed

selection : list
List of str, selecting which maps to make.
Available: 'counts', 'aeff', 'background', 'edisp'
By default, all spectra are made.

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

all products/quantities. Technically they are not all spectra.

This comment has been minimized.

Copy link
@adonath

adonath Oct 16, 2019

Author Member

Fixed.

return solid_angle[mask].sum()

def make_counts(self, observation):
"""Make counts

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

Make counts spectrum

return counts

def make_background(self, observation):
"""Make background

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

Make background spectrum


@lazyproperty
# TODO: move this to a RegionGeom class
def region_solid_angle(self):

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 16, 2019

Contributor

these functions are more helper functions for regions than members of the SpectrumDatasetMaker. In particular they might be used for the ReflectedRegionsBackgroundMaker as well.

This comment has been minimized.

Copy link
@adonath

adonath Oct 16, 2019

Author Member

Removed for now. I think it actually belongs to a RegionGeom class or maybe as a short-term solution to CountsSpectrum.

@adonath adonath self-assigned this Oct 16, 2019
@adonath adonath added this to the 0.15 milestone Oct 16, 2019
@adonath adonath merged commit 0e38c84 into gammapy:master Oct 16, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 16 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191016.8 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
@adonath adonath changed the title Implement `SpectrumDatasetMaker` Implement SpectrumDatasetMaker Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.