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

Improve map and spectrum events fill methods #2468

Merged
merged 3 commits into from Oct 17, 2019
Merged

Conversation

@cdeil
Copy link
Member

cdeil commented Oct 17, 2019

This PR makes the methods to fill events into Map or CountsSpectrum a bit better.

At least IMO, it's a bit more consistent this way.

Before, we had fill_map_counts as a function, and CountsSpectrum.fill as a method. Now, it's container.fill_events in both cases with the same API.

Before we had events. _get_coord_from_geom as private, but were accessing it from the outside in fill_map_counts. Now, it's events.map_coord and part of the public API, simiilar to other properties events already has.

@cdeil cdeil added the cleanup label Oct 17, 2019
@cdeil cdeil added this to the 0.15 milestone Oct 17, 2019
@cdeil cdeil added this to To do in gammapy.maps via automation Oct 17, 2019
@cdeil cdeil added this to To do in gammapy.spectrum via automation Oct 17, 2019
Copy link
Member

adonath left a comment

Thanks @cdeil! Please make return-type, name and docstring of EventList.map_coord() consistent.

The other comments describe my personal preference to just use the pattern Map.fill_by_coord(events.map_coord), instead of .fill_events. Please think about it once more again. However I won't object to .fill_events() either, I guess it's a common use-case explicitly defined in API...

gammapy/data/event_list.py Show resolved Hide resolved
@@ -481,19 +481,20 @@ def check(self, checks="all"):
checker = EventListChecker(self)
return checker.run(checks=checks)

def _get_coord_from_geom(self, geom):
"""Return MapCoord of the EventList relative to an input geometry.
def map_coord(self, geom):

This comment has been minimized.

Copy link
@adonath

adonath Oct 17, 2019

Member

I think the EventList API would be more consistent, if we don't pass geom and make map_coord a property just as .time, .offset, .skycoord. Just bundle all relevant coordinates and avoid copies if we are concerned about efficiency...

This comment has been minimized.

Copy link
@cdeil

cdeil Oct 17, 2019

Author Member

Did you see this?

https://github.com/gammapy/gammapy/pull/2468/files#diff-0e905bc71b734b5e047ddf9dce8d2ef6R59-R64

@registerrier already implemented logic to match events table to arbitrary map axis, e.g. for EVENT_ID, if the names match in a case insensitive way.

Removing the geom argument here would remove that feature, or reduce it to a hard-coded preset list of coords.

So I'll leave as-is for now, I still think this PR is a step in the right direction.
But yes, possibly this is something that could be changed in the future, probably we should only decide once we have real-world use cases (i.e. a data reduction and fittting working for maps with a TIME or EVENT_ID axis).

IF the conclusion is that geom shouldn't be passed to events.map_coord, then I agree that map.fill_events isn't useful any more - it'll be easy to replace, because you can just right-click in PyCharm and say "list callers" or use grep.

@@ -771,6 +771,10 @@ def interp_by_pix(self, pix, interp=None, fill_value=None):
"""
pass

def fill_events(self, events):

This comment has been minimized.

Copy link
@adonath

adonath Oct 17, 2019

Member

I'm still 48:52 to not add .fill_events() and just use Map.fill_by_coord(events.map_coord) otherwise one could argue why we don't implement .fill_bkg(), fill_psf(), etc. as well...

This comment has been minimized.

Copy link
@cdeil

cdeil Oct 17, 2019

Author Member

We do have this for PSF:

if isinstance(kernel, PSFKernel):

For BKG - there's no simple object such that map.fill_bkg(bkg) would make sense.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Oct 17, 2019

@adonath - I changed to return a MapCoord in 64cc992 .

It currently doesn't work. I think there's a bug in MapCoord.create - it doesn't take the frame information when skycoord is passed? @adonath - Can you please have a look and fix this issue either here or in a separate PR if you want a clean changelog for fixes?
https://gist.github.com/cdeil/7d8f1e26444d2479e1989f837a09d0ae

@cdeil cdeil force-pushed the cdeil:events-fill branch from 64cc992 to de7bbf5 Oct 17, 2019
@cdeil cdeil merged commit d03a32b into gammapy:master Oct 17, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 8 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191017.23 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
gammapy.maps automation moved this from To do to Done Oct 17, 2019
gammapy.spectrum automation moved this from To do to Done Oct 17, 2019
@cdeil cdeil assigned cdeil and unassigned adonath Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
2 participants
You can’t perform that action at this time.