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

Add function for HiPS to HEALPix conversion #132

Closed
wants to merge 3 commits into from

Conversation

adl1995
Copy link
Member

@adl1995 adl1995 commented Jul 28, 2018

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?

@cdeil cdeil added this to the 0.3 milestone Jul 28, 2018
@cdeil
Copy link
Contributor

cdeil commented Jul 28, 2018

@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 hips_to_healpix_array you introduce here basically does the same thing as the existing hips_tile_healpix_ipix_array

def hips_tile_healpix_ipix_array(shift_order: int) -> np.ndarray:

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.


return healpix_to_xy, xy_to_healpix

def hips_to_healpix(hips_url: str, npix: int, hpx_output_path: str):
Copy link
Contributor

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.

healpix_map = np.empty(hp.nside2npix(2 ** 12), dtype=np.double) # 2 ** 12?
healpix_map[:] = hp.pixelfunc.UNSEEN

for ipix in range(npix):
Copy link
Contributor

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:

def hips_tile_healpix_ipix_array(shift_order: int) -> np.ndarray:

except:
continue

# BSCALE and BZERO.
Copy link
Contributor

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):
Copy link
Contributor

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.

@cdeil
Copy link
Contributor

cdeil commented Jul 28, 2018

@adl1995 - I think @adonath only wanted to extend the HEALPix -> HipS direction to also work for PNG and JPEG, so most likely this PR doesn't overlap with that and you can just go ahead if you have time.

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.
@adl1995
Copy link
Member Author

adl1995 commented Aug 22, 2018

@cdeil - Sorry for getting back to this so late.

I have updated the hips_to_healpix function to generate the HEALPix map from a list of HiPS tiles. However, I'm not sure how to add a correct test case for this.

In the current test case test_hips_to_healpix there is initially a healpix_data array which contains numbers from 0 till 191. The returned healpix_map from hips_to_healpix function is of size 201326592, which is computed using hp.nside2npix(2 ** 12).

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?

@cdeil cdeil assigned cdeil and unassigned adonath Dec 13, 2018
@cdeil
Copy link
Contributor

cdeil commented Jan 8, 2019

I'll start a new branch to develop this functionality.

@cdeil cdeil closed this Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants