michitaro / healpix

An implementation of HEALPix in JavaScript / TypeScript
MIT License
14 stars 4 forks source link

Add nside2order and rename uniq ID scheme functions #16

Closed cdeil closed 6 years ago

cdeil commented 6 years ago

This PR is an attempt to improve the uniq related functions:

Overall the goal is to have clearer and more consistent function and variable names with the rest of the package. Looking at the old encode_id and decode_id, I didn't know what they were. Having a new type HealpixId that's just number and using is only here, but not in all the other places where ipix is used doesn't seem useful.

I wonder if the implementation of these methods can be improved. Is there a reason to keep the bitshifting and wile loop in uniq2orderpix for performance reasons? Or would calling math.log2 and maybe also Math.pow like in the Python implementation by @tboch in https://github.com/cds-astro/mocpy/blob/master/mocpy/utils.py#L4 be better? Another alternative could be to add an ilog2 that's implemented in a similar way as in Python here?

@tboch - Thoughts?

Does someone already have a good set of unit test cases that we should add?

michitaro commented 6 years ago

IE doesn't have Math.log2. If we make some polyfill for Math.log2, it would be some thing like this Math.log(x) / Math.LN2. In such case I'm not sure it will have enough precision. This is why I didn't use Math.log2.

Of course, after ensuring that Math.log(x) / Math.LN2 has enough precision for our cases, there is no reason not to use Math.log2.

michitaro commented 6 years ago

I updated the last comment. I submitted by mistake.

cdeil commented 6 years ago

For nside2order where we need ilog2 we could also use nside.toString(2).length? https://stackoverflow.com/questions/9939760/how-do-i-convert-an-integer-to-binary-in-javascript

The we avoid the call to Math.log2 and this PR can go in? (and possible further improvements to the uniq2orderpix implementation could come later if someone finds a good way to do it)

michitaro commented 6 years ago

The we avoid the call to Math.log2 and this PR can go in?

Yes!

In my environment, nside.toString(2).length is bit slower. I prefer loop & bit-shifting one https://jsfiddle.net/k6u9oxs5/10/

ilog2_1: 18ms
ilog2_2: 205ms
Math.log2: 24ms
ilog2_1: 27ms
ilog2_2: 170ms
Math.log2: 15ms
cdeil commented 6 years ago

I added ilog2 as you suggest in c0c9b36 .

michitaro commented 6 years ago

Thanks!