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 select_region method for event lists #2254

Merged
merged 6 commits into from Jun 24, 2019

Conversation

@registerrier
Copy link
Contributor

@registerrier registerrier commented Jun 21, 2019

Following PIG 10, see #2129 this PR introduces an EventListBase.select_region(region, wcs) as well as some tests.

This will be used for PR #2089 to properly select events.

@registerrier registerrier added this to the 0.13 milestone Jun 21, 2019
@registerrier registerrier requested a review from adonath Jun 21, 2019
@cdeil cdeil changed the title Add EventListBase.select_region and tests Add select_region method for event lists Jun 24, 2019
@cdeil cdeil self-assigned this Jun 24, 2019
@cdeil cdeil added this to To do in gammapy.data via automation Jun 24, 2019
Copy link
Member

@cdeil cdeil left a comment

@registerrier - Thanks!

The proposal in the regions PIG (see https://docs.gammapy.org/dev/development/pigs/pig-010.html#region-arguments ) was to take a single region, not a list of regions. (and users would be responsible to make compound regions if that's what they want before calling)

Also the suggestion was to call make_region on the first line of such methods, which is already available: https://docs.gammapy.org/dev/api/gammapy.utils.regions.make_region.html

Is there a good reason you're not doing that here? If no, could you please change to this pattern?

I would like to avoid having a mix of methods that take a single region or a list of regions, or having to support and test both everywhere. If we support list of regions as input, it should be done in one place in a generic way, in the make_region helper function. We might if there's a real reason, but a priory we should just implement the plan from the PIG, no?

Also, you need to add wcs in the parameters list in your docstrings.

Then this PR can go in.

@registerrier
Copy link
Contributor Author

@registerrier registerrier commented Jun 24, 2019

OK. I am implementing the change.
One question though. At the moment make_region(region) can return anything if region is neither a str nor a SkyRegion. Wouldn't it be safer to raise an exception if region is neither a str nor a SkyRegion/PixelRegion?

@cdeil
Copy link
Member

@cdeil cdeil commented Jun 24, 2019

@registerrier - Yes, make_region should end like make_pixel_region, i.e. like this:

 elif isinstance(region, Region):
        return region
    else:
        raise TypeError("Invalid type: {!r}".format(region))

Copy link
Member

@adonath adonath left a comment

Thanks @registerrier! I've left a few minor comments...

"""
region = make_region(region)
position = self.radec
mask = np.where(region.contains(position, wcs))[0]
Copy link
Member

@adonath adonath Jun 24, 2019

I think there is no need for the call np.where(), just remove and remove a boolean array. Or is there an advantage in having the indices?

Copy link
Contributor Author

@registerrier registerrier Jun 24, 2019

Right.
In reality, there is probably no need to have filter_region and select_region at the same time. This is copied from the filter_circular_region and select_circular_region logic. But this is probably irrelevant. filter_circular_region is never used in the code.

@@ -379,6 +380,45 @@ def filter_circular_region(self, region):

return mask

def select_region(self, region, wcs):
"""Select events in circular regions.
Copy link
Member

@adonath adonath Jun 24, 2019

I think circular is not true here, it's more general. Maybe change to ...in specified region.?

Copy link
Contributor Author

@registerrier registerrier Jun 24, 2019

done

return self.select_row_subset(mask)

def filter_region(self, region, wcs):
"""Create selection mask for event in given circular regions.
Copy link
Member

@adonath adonath Jun 24, 2019

"event in given circular regions" -> "events in given region."

Copy link
Contributor Author

@registerrier registerrier Jun 24, 2019

done

region : `~regions.SkyRegion` or str
Sky region or string defining a sky region
wcs : `~astropy.wcs.WCS`
the world coordinate system transformation to assume
Copy link
Member

@adonath adonath Jun 24, 2019

Start upper case...

Copy link
Contributor Author

@registerrier registerrier Jun 24, 2019

done

@registerrier
Copy link
Contributor Author

@registerrier registerrier commented Jun 24, 2019

I propose we adapt/remove the filter_region and filter_circular_region at the same time, once we decide what we do.
The travis CI errors are unrelated.
Merging now.

@registerrier registerrier merged commit d635a74 into gammapy:master Jun 24, 2019
8 of 9 checks passed
gammapy.data automation moved this from To do to Done Jun 24, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 24, 2019

@registerrier - Small follow-up commit in master in 880b103 .

filter region is not a good name for a method that creates a mask. We should probably rename to region_select_mask so that it shows up in the API docs right next to the closely related region_select method. Or just remove those filter methods, because it's just two lines and thus not very useful, the few callers that need it can just put those two lines.
On the other hand, there are valid use cases where one needs the mask and then do other things with the mask than to apply it for row selection.

filter_circular_region is never called or tested and can just be deleted?

@registerrier @adonath - What's your preference concerning improving or removing the "filter" methods?

@adonath
Copy link
Member

@adonath adonath commented Jun 25, 2019

@cdeil I have a slight preference to just remove the filter_... methods. Numpy masking is really fundamental and I think it's OK for users to do this themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.data
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants