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

Adapt MapMakerObs to return a MapDataset #2375

Merged
merged 3 commits into from Sep 19, 2019

Conversation

@AtreyeeS
Copy link
Member

commented Sep 18, 2019

This PR adapts MapMakerObs to return a MapDataset instead of a dictionary.
This needs some adaption in MapMaker and MapMakerRing as well. Since these classes will move away in near future, I have put in some temporary ugly lines.

@AtreyeeS AtreyeeS requested a review from adonath Sep 18, 2019
Copy link
Member

left a comment

Thanks a lot @AtreyeeS! I agree this is a useful change, I've left some comments with suggestions to improve the code. Once those are addressed, the PR should be ready to merge.

if name == "exposure":
data = maps_dataset.exposure.quantity.to_value(maps[name].unit)

This comment has been minimized.

Copy link
@adonath

adonath Sep 18, 2019

Member

There is no need for the unit conversion here. Quantities are handled correctly in .fill_by_coord() see https://github.com/gammapy/gammapy/blob/master/gammapy/maps/wcsnd.py#L221.

maps[name].fill_by_coord(obs_maker.coords_etrue, data)
else:
if name == "counts":
data = maps_dataset.counts.quantity.to_value(maps[name].unit)

This comment has been minimized.

Copy link
@adonath

adonath Sep 18, 2019

Member

Same comment...

data = maps_dataset.counts.quantity.to_value(maps[name].unit)
maps[name].fill_by_coord(obs_maker.coords, data)
if name == "background":
data = maps_dataset.background_model.map.quantity.to_value(

This comment has been minimized.

Copy link
@adonath

adonath Sep 18, 2019

Member

Same comment...

@@ -268,9 +275,30 @@ def run(self, selection=None):
for name in selection:
getattr(self, "_make_" + name)()

background_model = BackgroundModel(self.maps["background"])

if "psf" in self.maps:

This comment has been minimized.

Copy link
@adonath

adonath Sep 18, 2019

Member

As self.maps is a dictionary the if and else blocks could be removed and replaced by psf = self.maps.get("psf")

exposure=self.maps["exposure"],
background_model=background_model,
psf=psf,
edisp=edisp,

This comment has been minimized.

Copy link
@adonath

adonath Sep 18, 2019

Member

As there is the access to the self.observation objects, you could also copy over the GTI table here and maybe set the name of the dataset to "obs-{}".format(observation.obs_id) or similar.

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Sep 18, 2019

Author Member

Thanks @adonath ! Addressed the changes...

@adonath adonath self-assigned this Sep 19, 2019
@adonath adonath added this to the 0.14 milestone Sep 19, 2019
@adonath

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Thanks a lot @AtreyeeS! Once we have the MapDataset.stack() method implemented we should consider adapting our tutorials to directly use the MapMakerObs and maybe also rename it then...

@adonath adonath merged commit edf9db3 into gammapy:master Sep 19, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 2 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190918.16 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
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.