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

Clean up sky region select code #2262

Merged
merged 7 commits into from Jul 2, 2019
Merged

Clean up sky region select code #2262

merged 7 commits into from Jul 2, 2019

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Jun 28, 2019

This PR is an attempt to follow up on #2254 (comment) and to remove and clean out the sky circle filter methods.

The difficulty is what to do in the cases where no WCS is available.

This PR introduces one possible approach, a class SphericalCircleSkyRegion which subclasses CircleSkyRegion and redefines contains to use spherical distance instead of the planar approximation.

I don't know if this is the best approach or if we should do something else. I've put a TODO comment with this question.

@registerrier @adonath - Thoughts?

Concretely: merge this now and try out the SphericalCircleSkyRegion for a bit and see if we like it or not? Or do you have another suggestion how to proceed for now?

@cdeil cdeil added the cleanup label Jun 28, 2019
@cdeil cdeil added this to the 0.13 milestone Jun 28, 2019
@cdeil cdeil requested review from adonath and registerrier Jun 28, 2019
@cdeil cdeil self-assigned this Jun 28, 2019
Copy link
Member

@adonath adonath left a comment

Thanks a lot @cdeil! Looks good to me. Do you have a proposal what to do with https://github.com/gammapy/gammapy/blob/master/gammapy/catalog/utils.py#L40 and https://github.com/gammapy/gammapy/blob/master/gammapy/catalog/utils.py#L93?

I think it's also needed to adapt the first_steps and pulsar_analysis tutorial, which make both use of the select_sky_cone method.

@@ -99,3 +99,23 @@ def make_pixel_region(region, wcs=None):
return region
else:
raise TypeError("Invalid type: {!r}".format(region))


class SphericalCircleSkyRegion(CircleSkyRegion):
Copy link
Member

@adonath adonath Jun 28, 2019

I guess this class is to support the case of the circular region, where no WCS is needed, with a uniform API. I think it's a good idea. So +1 to keep, but I agree the better place would be the regions package...

Copy link
Contributor

@registerrier registerrier left a comment

Thanks @cdeil . I have mixed feelings. Isn't adding a specific region a bit confusing for the user?

A general question whether we want to do any region based event selection without a wcs. I think it will be very cumbersome to have the two categories at the same time.
For instance, if we are to use event filtering with a FoV selection using ObservationFilter to make Maps we have a problem because IRF maps such as exposure have to be binned on the wcs. So the filtering should be done using the wcs. Otherwise the spatial filtering becomes an irrelevant feature, no?

This is different if you consider more general selections like e.g. on DataStoreObservation. There regions on the Sky should be accessible.

Anyway, ideally this feature should reside in regions, indeed. Either in the form of a specific class or with the ability to call with wcs=None for CircleSkyRegion.

Otherwise, I am perfectly fine with the change to remove filter_region.

Note that there is still one notebook using select_sky_region (pulsar analysis).

@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 28, 2019

@registerrier - I didn't want to add SphericalCircleSkyRegion, it was just the simplest solution I could think of to make the two callers to the previous methods work, where no WCS is available.

I think as soon as maps are involved, these filter methods should never be called. We did that a few years ago, and then got artifacts at the edge of the FOV because a geometric circle isn't the same as a pixelised circle, some pixels are partly in the region. The solution there is to always make a mask and multiply with it to select.

@adonath - the catalog utils helper functions are still called by the obs table select methods. I will remove them in a follow-up PR where I change to select_region there. Whatever solution we come up with for events, we would also use for observations, so it seemed easier to first figure out what to put first for events only (and I had to go home).

We can propose changes and additions in the regions package, but that will require some discussion, it will not be available immediately. What we put in this PR doesn't have to be the perfect solution, we can just change further next week.

Realistically I think either we merge this, or I try and change the tests / callers to have a WCS and pass it down. @registerrier - Which solution would you prefer? I'm still not sure how to proceed.

@cdeil cdeil changed the title Clean up events sky circle filter methods Clean up sky region select code Jul 1, 2019
@cdeil
Copy link
Member Author

@cdeil cdeil commented Jul 1, 2019

I've continued here a bit and removed the cone and box select in gammapy.catalog.utils, and adapted the one caller for the obs table select. I removed the box select feature for now, and simplified the circle select.

The obs table spatial select API is still kept here. One this goes in, I would open a follow-up issue to discuss whether to change the whole obs_table select (or whether to replace it completely with just using a Table and helper functions).

I think the pulsar phase notebook is broken, there is no spectrum at the end? Is that issue introduced here or already present in master? If you could have a look at that specifically, that would be useful.

@adonath (and @registerrier if you're available) - please review.

Copy link
Contributor

@registerrier registerrier left a comment

Thanks @cdeil . Good to go.

@cdeil cdeil merged commit 912e64d into gammapy:master Jul 2, 2019
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants