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

Allow non-power-of-two nside values when the pixel scheme is RING #584

Merged
merged 6 commits into from
Nov 13, 2019
Merged

Allow non-power-of-two nside values when the pixel scheme is RING #584

merged 6 commits into from
Nov 13, 2019

Conversation

tmolteno
Copy link

@tmolteno tmolteno commented Nov 4, 2019

Hi There,

This PR changes the isnsideok() function to allow non-power-of-two nside values (when using the RING scheme). This is useful when searching for a pixelization scheme that is just high enough - which can lead to huge efficiencies in the situation where one is discretizing a small subset of the sphere at high resolution.

The test is somewhat application specific (to suit my needs) so apologies in advance for that.

Refers to Issue #583.

Tim

Copy link
Member

@zonca zonca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thank you very much!
just 1 minor point

@@ -344,8 +344,11 @@ cdef pixset_to_array(rangeset[int64] &pixset, buff=None):
ii += 1
return ipix

cdef bool isnsideok(int nside):
return nside > 0 and ((nside & (nside -1))==0)
cdef bool isnsideok(int nside, bool nest):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about having nest default to False?
That is common in all other functions that accept that argument.

@zonca
Copy link
Member

zonca commented Nov 12, 2019

also, there is another isnsideok method in pixelfunc.py...https://github.com/healpy/healpy/blob/master/healpy/pixelfunc.py#L1160
can you remove that so we always use the one defined in cython?

@tmolteno
Copy link
Author

Hi There. I will make the default argument as you suggest. Good point about the duplicate in pixelfunc. Will submit another PR soon.

@tmolteno
Copy link
Author

I have left the version in pixelfunc (suitably modified) as it seems a little more logical for isnsideok to be in the pixel functions (happy to shift it if necessary)

@zonca
Copy link
Member

zonca commented Nov 12, 2019

there is actually another one in pixelfunc.py https://github.com/healpy/healpy/blob/master/healpy/pixelfunc.py#L1160, can you remove that one as well?

@tmolteno
Copy link
Author

Wow, there were three of them! The isnsideok() in https://github.com/healpy/healpy/blob/master/healpy/pixelfunc.py#L1160 is exported to the healpy module. Didn't see it there at all as I had my eyes down looking inside the .pyx files. I will leave that one and remove the one I modified in _pixelfunc.pyx (which has identical functionality, but has much uglier documentation.)

@zonca
Copy link
Member

zonca commented Nov 13, 2019

Thanks!

@zonca zonca merged commit 80653a7 into healpy:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants