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 overlap option for cone search #67

Open
cdeil opened this issue Oct 17, 2017 · 4 comments
Open

Add overlap option for cone search #67

cdeil opened this issue Oct 17, 2017 · 4 comments
Assignees

Comments

@cdeil
Copy link
Member

cdeil commented Oct 17, 2017

In Gammapy we currently call healpy.query_disc with both the inclusive=True and the inclusive=False option (see gammapy/gammapy#1167 (comment)).

As discussed in #41 , probably we don't want to spend the time to fully implement healpy.query_disc concerning the inclusive=False behaviour.

But maybe we could add an option to HEALPix.cone_search_lonlat that corresponds to the inclusive=True behaviour, i.e. restricts the pixels to the ones where the center is in the cone?

@astrofrog - OK to add this?

How about calling it overlap={'center', 'full'} which is maybe a bit more intuitive than inclusive = {True, False} and also more extensible should we want to add other options in the future?
Concerning implementation: should it just be done at the end of the function in an if overlap == 'center' block using a Numpy expression, subsetting with a mask that's true if the pixel center is in the cone? Or would you suggest this be added in the C or Cython code somehow?

@cdeil cdeil added this to the 0.3 milestone Oct 17, 2017
@astrofrog
Copy link
Member

I agree with this - I would maybe call the options overlap={'center', 'any'} because full to me sounds like the pixel has to overlap fully whereas really any overlap will do.

Implementation-wise, doing it in core.py in Python/Numpy is fine - if we want to avoid making temporary arrays then doing it in Cython would be better, but honestly I think just doing it in core.py would be fine for now and we can optimize later?

@tboch
Copy link

tboch commented Oct 17, 2017

I agree with this - I would maybe call the options overlap={'center', 'any'} because full to me sounds like the pixel has to overlap fully whereas really any overlap will do.

+1 for using 'center' and 'any', sounds easier to understand to me

@cdeil
Copy link
Member Author

cdeil commented Oct 17, 2017

Agreed that "full" was a complete misnomer and that overlap="any" is good.

Implementation-wise, doing it in core.py in Python/Numpy is fine - if we want to avoid making temporary arrays then doing it in Cython would be better, but honestly I think just doing it in core.py would be fine for now and we can optimize later?

Agreed. I'll try to find time to make a pull request later this week.

@cdeil cdeil self-assigned this Oct 17, 2017
@cdeil cdeil modified the milestones: 0.3, 0.4 Oct 24, 2018
@taldcroft
Copy link
Member

Discussed at astropy coordination meeting in Tucson. 👍 from @taldcroft and @adrn.

@cdeil cdeil modified the milestones: 0.4, 0.5 Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants