Clean up sky region select code

[cdeil]

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?

[adonath]

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.

[adonath]

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...

[registerrier]

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]

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

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.

[registerrier]

Thanks @cdeil . Good to go.