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 function to compute counts maps #1317

Merged
merged 14 commits into from Mar 27, 2018

Conversation

Projects
None yet
3 participants
@registerrier
Contributor

registerrier commented Feb 16, 2018

This PR is a follow up of some [comments (https://github.com//pull/1314#pullrequestreview-97012013)
made during the review of PR #1314.

This introduces a MapAxis.type which is selected depending on the MapAxis.unit.
Checking on the unit might be ugly, but it ensures consistency between axis type and the unit used.

Only 2 types are defined for now: energyand time. The rest is any.

As far as I understand spatial axes are not MapAxis in maps.

Would that be OK?

@registerrier registerrier added this to the 0.8 milestone Feb 16, 2018

@registerrier registerrier requested review from cdeil and woodmd Feb 16, 2018

@woodmd woodmd referenced this pull request Feb 19, 2018

Merged

Improve MapCoord interface #1318

@woodmd

This comment has been minimized.

Member

woodmd commented Feb 19, 2018

This looks good overall. I've just opened #1318 with some improvements to the coordinate-based maps interface which I think could simplify the logic in fill_map_counts (for instance automatically handling coordinate frame conversions). I suggest that we work on merging #1318 first before finishing this PR.

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 23, 2018

@registerrier - This PR contains the commits from #1314 as well. How do you want to handle this, i.e. get your changes in?

Up to you. I think it might be easiest if you just make one PR and close the other and we aim to get it merged today.

One comment I have is that I don't see how make_map_counts is useful, if it's just two short lines, suggest to remove it.

Note that @woodmd improved the map coord interface in #1318. I didn't look at this again here, but it might be useful to use MapCoord for this?

Let's discuss on Skype later today if you're available.

else:
# should raise an error here. The map is not correct.
raise ValueError("Incorrect coordsys of input map.")

This comment has been minimized.

@woodmd

woodmd Feb 23, 2018

Member

There's no need to convert to the coordinate system of the map here. If you're passing a SkyCoord the coordinate frame will be automatically matched to that of the map.

tmp.append(event_list.table[axis.name.lower()].to(axis.unit))
else:
raise ValueError("Cannot find MapGeom axis {} in EventList", axis.name)

This comment has been minimized.

@woodmd

woodmd Feb 23, 2018

Member

If you map has consistent axis names you could use the new dict-based access pattern here, e.g.:

tmp = {}
tmp['skycoord'] = event_list.radec
tmp['energy'] = event_list.energy.to(axis.unit)
ndmap.fill_by_coords(tmp)
the input event list
nd_map : `~gammapy.maps.WcsNDMap`
Target map
"""

This comment has been minimized.

@woodmd

woodmd Feb 23, 2018

Member

I think you could have this method accept a Map argument instead of WcsNDMap. There's nothing you're doing here that specifically relies on the map being WCS-based.

This comment has been minimized.

@registerrier

registerrier Feb 23, 2018

Contributor

Right. I changed the docstring accordingly.
Note that the code that was committed earlier was not the right one because of a mix-up with between branches.

@cdeil

@registerrier - I left some inline comments. This is starting to look good!

"""
# The list will contain the event table entries to be fed into the WcsNDMap
coord_dict = dict()
ref_geom = ndmap.geom

This comment has been minimized.

@cdeil

cdeil Mar 1, 2018

Member

Remove this local variable? It's just used once, and the line there isn't very long yet.

----------
event_list : `~gammapy.data.EventList`
the input event list
ndmap : `~gammapy.maps.Map`

This comment has been minimized.

@cdeil

cdeil Mar 1, 2018

Member

Here you use ndmap, but I think the code should also work for the sparse maps, i.e. we don't want "nd" in the argument name here. On the other hand, just using "map" has the disadvantage that we're clobbering the Python built-in "map" function, which is not recommended, by default flagged by Python linters.
We will have that issue everywhere. I'm not sure what is best here. @woodmd @registerrier ?

# Now check the other axes and find corresponding entries in the EventList
# energy and time are specific types
# TODO: add proper extraction for time

This comment has been minimized.

@cdeil

cdeil Mar 1, 2018

Member

Move this TODO down below the energy handling? That would be the location where to put the time handling special code, no?

coord_dict.update({axis.name: event_list.energy.to(axis.unit)})
# We look for other axes name in the table column names (case insensitive)
else:
try:

This comment has been minimized.

@cdeil

cdeil Mar 1, 2018

Member

Please simplify as we discussed.

ndmap.fill_by_coord(coord_dict)
def make_map_counts(event_list, ref_geom, meta=None):

This comment has been minimized.

@cdeil

cdeil Mar 1, 2018

Member

I'd suggest to just put "geom", it's clear that "make_map_counts" with a "geom" passed in should make a map with that geometry, no?
Let's decide here about "map" and "geom" argument names in this PR, and then try to be consistent everywhere in Gammapy as much as possible.

@@ -417,6 +425,11 @@ def unit(self):
"""Return coordinate axis unit."""
return self._unit
@property
def type(self):

This comment has been minimized.

@cdeil

cdeil Mar 1, 2018

Member

Please add a test for this type, or ideally even one that covers the three cases of "energy", "time" and "any".

@cdeil cdeil changed the title from Mapaxis type to Add function to compute counts maps Mar 16, 2018

@cdeil cdeil self-assigned this Mar 26, 2018

@cdeil

@registerrier - I did some minor clean-up in 1cc7d61 (mostly auto code format in PyCharm and some things PyCharm points out).

I think we should merge this now if CI passes. But then later, the two TODOs should be implemented, to handle time axis in a special way, and to put a better test case with an event list where it's clear from reading the test what to expect.

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 27, 2018

There's missing @requires_data('gammapy-extra') markers, and for the HEALPix test also @requires_dependency('healpy') in the tests.
See e.g. https://ci.appveyor.com/project/cdeil/gammapy/build/1.0.3205/job/oo8sujhmbr9iw3x6#L2480

I'll add those now...

@cdeil cdeil dismissed their stale review via 995bab3 Mar 27, 2018

@cdeil cdeil merged commit 5240db4 into gammapy:master Mar 27, 2018

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
@cdeil

This comment has been minimized.

Member

cdeil commented Mar 27, 2018

@registerrier - Thanks!

@cdeil cdeil referenced this pull request Apr 12, 2018

Merged

Improve 3D analysis code using gammapy.maps #1373

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