michitaro / healpix

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

Clarify x/y vs p/q #11

Open cdeil opened 6 years ago

cdeil commented 6 years ago

@michitaro - I'm still in the process of reading the HEALPix papers, using your code to try to fully understand.

This list of variable names and descriptions is very helpful: https://github.com/michitaro/healpix/blob/master/src/index.ts#L4

You do sometimes use x/y, but those are not defined in the list. Note that you have tu2fpq and fxy2tu, so maybe x/y and p/q are the same?

Is it possible to make the notation more consistent; or to clarify what x/y are in that list?

michitaro commented 6 years ago

I have added note about x and y. https://github.com/michitaro/healpix/commit/2d3aaef81585c6a85f4979f5639e56206ec78273

Note that you have tu2fpq and fxy2tu, so maybe x/y and p/q are the same?

Both x/y and p/q are along with north-west/north-east axis, but p/q run on [0, 1] while x/y run on [0, nside).

cdeil commented 6 years ago

Is it useful / needed to have both x/y and p/q?

tu2fpq is only called once, in tu2fpq:

export function tu2fxy(nside: number, t: number, u: number) {
    const { f, p, q } = tu2fpq(t, u)
    const x = clip(Math.floor(nside * p), 0, nside - 1)
    const y = clip(Math.floor(nside * q), 0, nside - 1)
    return { f, x, y }
}

And you're not offering a fpq2tu.

Is this extra clip in tu2fxy really needed?

If it's possible without loosing functionality, I'd suggest removing the p/q coordinate system and moving the content of tu2fpq into tu2fxy and only use fxy. (the other way would also be possible, but fxy is used in many places, seems the preferred coordinates in this package.

michitaro commented 6 years ago

I'd suggest removing the p/q coordinate system and moving the content of tu2fpq into tu2fxy and only use fxy.

OK. let's do so.

Is this extra clip in tu2fxy really needed?

Ah..., they seem to be not needed. I was caring the case where p == 1 or q ==1, but p and q always < 1. It's OK to remove them.