michitaro / healpix

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

Expose more functions and describe algorithm #14

Closed cdeil closed 6 years ago

cdeil commented 6 years ago

This is a first attempt to describe the algorithm used in the API docs. Please check, my understanding of what is going on is still not very good.

Note that it mentions tu and fxy and exposes the functions to compute those. Is this a good direction, or would you prefer those not to appear in the API docs?

One specific question is why you have chosen t = x_s and u = y_s / pi when compared to the paper. I.e. why not u = y_s to be closer aligned to the paper? I haven't checked yet how the FITS WCS standard HEALPix projection compares, i.e. whether it matches (x_s, y_s) or (t, u) exactly or not.

michitaro commented 6 years ago

One specific question is why you have chosen t = x_s and u = y_s / pi when compared to the paper.

I don't remember why I did so, but there is no significant reason. I simply chose shorter notation maybe.

It might be better that we replace t -> x_s, u->y_s. If you would like notations consistent with the paper, of course you can replace them.

Should I merge it now? or after replacing them?

cdeil commented 6 years ago

I think the current tu is nice because it gives shorter variable / function names.

I just meant specifically for u = y_s / pi why you chose to use the factor pi like that when defining u. It was just a question. If you think it doesn't matter, suggest to leave as-is. If you think changing the factor pi is a little bit better, I'd be happy to make the change here or in a follow-up PR.

michitaro commented 6 years ago

Ah..., I know the factor pi is unnecessary. I would appreciate it if you could change them.

cdeil commented 6 years ago

I compared your package against the WCS projection (see https://github.com/astropy/astropy-healpix/issues/99) and found this:

const lon = - 148 / 180 * Math.PI;
const lat = 65 / 180 * Math.PI;
const z = Math.sin(lat);
const a = lon;
const {t, u} = za2tu(z, a)
console.log('x = ', t * 180 / Math.PI)
console.log('y = ',  u * 180 / Math.PI)
x =  -99.60716127015343
y =  66.14250235769997

The Y value is in agreement with the HEALPix projection in Astropy / WCSLib / http://adsabs.harvard.edu/abs/2007MNRAS.381..865C, but the X-value doesn't match.

So it seems the u was always correct, and just the range of values in the description not. This is fixed in 262f377

I'm not sure about the difference in X yet, probably this would require more substantive changes in many places of the code to bring in line with the "official" definition of the projection?

@michitaro - Can you merge this PR and then you or I look a that separately?

michitaro commented 6 years ago

Thank you very much for the inspection.

I'm not sure about the difference in X yet, probably this would require more substantive changes in many places of the code to bring in line with the "official" definition of the projection?

I'll check them too.

cdeil commented 6 years ago

I think the difference in X is just due to different ranges for lon? You map lon=1 deg to x=1 deg, i.e. have it increase in x direction. I think this is how also Gorski defined it, and it's just Calabretta that reversed the X-axis for WCS?

So probably no action needed.

cdeil commented 6 years ago

Since I was using the example from https://github.com/substack/healpix#example as a test case, the other thing that confused me was the sign for y, to put y<0 for the northern hemisphere.

But I think that's very non-standard, both Gorski and Calabretta mapped the northern hemisphere lat > 0 to y > 0, and so do you.

michitaro commented 6 years ago

Yes, increasing of x increases lon, and y > 0 is for northern hemisphere in this package. No problem?

cdeil commented 6 years ago

All good!

michitaro commented 6 years ago

Your document is also updated. Thanks! https://michitaro.github.io/healpix/typedoc/modules/_index_.html