-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add function for HiPS to HEALPix conversion #132
Conversation
@adonath - Did you start coding up this functionality already? @adl1995 - I don't think @adonath did, but I'm not sure. If you want to go ahead with this, you will have to more closely study the implementation and test case at #123 for the direction HEALPix -> HiPS, and then do the other direction HiPS -> HEALPix as similar as possible. My understanding is that the Line 125 in 0b52667
and you should use that one. For the test case, one thing you could do is start with HEALPix, convert to HipS tiles (no need to write to disk) and convert back to HEALPix. The input and output should be identical. |
hips/draw/healpix.py
Outdated
|
||
return healpix_to_xy, xy_to_healpix | ||
|
||
def hips_to_healpix(hips_url: str, npix: int, hpx_output_path: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not mix computation with I/O.
I would suggest to just take the HEALPix pixels as a Numpy array:
https://hips.readthedocs.io/en/latest/api/hips.healpix_to_hips.html
This assumes that the HEALPix fits in memory, but that's the common use case, and supporting larger images requires fancy file I/O where only chunks of the image are memmapped. Let's not do that.
hips/draw/healpix.py
Outdated
healpix_map = np.empty(hp.nside2npix(2 ** 12), dtype=np.double) # 2 ** 12? | ||
healpix_map[:] = hp.pixelfunc.UNSEEN | ||
|
||
for ipix in range(npix): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not have a double for loop.
The function should take a list (or iterable) of HiPSTile as input, and only have a loop over that.
The way you pre-allocate the output array before the loop loops OK.
There is no second loop over pixels within a tile, because the operations for those pixels are vectorised as Numpy expressions:
Line 125 in 0b52667
def hips_tile_healpix_ipix_array(shift_order: int) -> np.ndarray: |
hips/draw/healpix.py
Outdated
except: | ||
continue | ||
|
||
# BSCALE and BZERO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not be concerned with BSCALE or BZERO here. A HipsTile has a property that gives you the data as a Numpy array.
@@ -36,3 +37,10 @@ def test_healpix_to_hips(tmpdir, file_format): | |||
|
|||
properties = (tmpdir / 'properties').read_text(encoding=None) | |||
assert file_format in properties | |||
|
|||
def test_hips_to_healpix(tmpdir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a simple test that doesn't use real data for two reasons:
- Hitting the network and disk is slow, and for the network error-prone
- Your test should end with an assert value on one or a few pixel values; if you take real data, it's not clear from looking at the test if the code works, you get an arbitrary value. If you set up a test case where you see what goes in from reading the test code, it's clear what should come out.
HEALPix pixels are now passed as a NumPy array. The test case is updated to covert HEALPix to HiPS and then convert the resulting HiPS tiles to a HEALPix map.
@cdeil - Sorry for getting back to this so late. I have updated the In the current test case Should these two arrays be identical for the function to be considered correct? I think I'm missing out on a step in the conversion process. Do you know if the HiPS paper goes into detail about this conversion process? |
I'll start a new branch to develop this functionality. |
Following issue #120, this pull request adds a function for generating a HEALPix map from HiPS.
The function is replicated from this GitHub Gist by @tboch: https://gist.github.com/tboch/f68cd1bb1529d8ac12184b40e54ba692
@cdeil - Can you please suggest some test cases for this?