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

Clarify x/y vs p/q #11

Open
cdeil opened this issue Jun 24, 2018 · 3 comments
Open

Clarify x/y vs p/q #11

cdeil opened this issue Jun 24, 2018 · 3 comments

Comments

@cdeil
Copy link
Contributor

cdeil commented Jun 24, 2018

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

@cdeil cdeil mentioned this issue Jun 25, 2018
@michitaro
Copy link
Owner

I have added note about x and y.
2d3aaef

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
Copy link
Contributor Author

cdeil commented Jun 29, 2018

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
Copy link
Owner

michitaro commented Jun 29, 2018

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.

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

No branches or pull requests

2 participants