search

hipspy / hips

Python library to handle HiPS
https://hips.readthedocs.io
13 stars 16 forks source link

Make improvements to compute_healpix_pixel_indices utility function #50

Closed adl1995 closed 7 years ago

adl1995 commented 7 years ago

Made improvements to the algorithm as outlined in #43.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.7%) to 85.41% when pulling 6a4655c84df03f2dd9987f1f015488bae0035724 on adl1995:improve_compute_healpix_pixel_indices into 84d60c18e2139a3e55a25d59c95198d6825648fe on hipspy:master.

cdeil commented 7 years ago

The results look good to me.

You have to update the function docstring. It currently describes the old algorithm (very briefly, with one sentence summary).

Concerning implementation, I have to admit that I find the current np.bincount followed by np.nonzero very hard to understand (even after looking at https://docs.scipy.org/doc/numpy/reference/generated/numpy.bincount.html). @adl1995 - Could you please try https://docs.scipy.org/doc/numpy/reference/generated/numpy.unique.html as suggested here? https://stackoverflow.com/a/25943480/498873 It should give identical results, be one line shorter/simpler and my guess is that it's also faster (although we shouldn't spend time on benchmarking that now, we'll do that once we have a complete package to find the bottlenecks: probably it's going to be the projective transform call in drawing)

@tboch - How do you want to handle updates to https://hips.readthedocs.io/en/latest/drawing_algo.html#naive-algorithm ? Would it be OK if @adl1995 updates that description to reflect the algorithm we're implementing? (e.g. that page currently still says that tiles should be split in triangles, and that query_disk is called)

adl1995 commented 7 years ago

@cdeil I have made the changes. Please check.

tboch commented 7 years ago

@cdeil - I'm fine with @adl1995 updating https://hips.readthedocs.io/en/latest/drawing_algo.html#naive-algorithm .

cdeil commented 7 years ago

OK, I'm merging this now so that you can more easily continue with your other PRs.

@tboch - If you ever find that I merged something too quickly, please just leave another comment or open a new issue.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.8%) to 85.366% when pulling f746d04e6a3cbe73c4c8ce6b8a32d3020c83bce5 on adl1995:improve_compute_healpix_pixel_indices into 84d60c18e2139a3e55a25d59c95198d6825648fe on hipspy:master.