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

Improve tile and allsky orientation handling #90

Merged
merged 4 commits into from
Jul 25, 2017
Merged

Conversation

cdeil
Copy link
Contributor

@cdeil cdeil commented Jul 24, 2017

This PR improves the all-sky image implementation and tests to also work with FITS images.

@cdeil cdeil added this to the 0.1 milestone Jul 24, 2017
@cdeil cdeil self-assigned this Jul 24, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.699% when pulling 40aaaaf on cdeil:allsky2 into fce110c on hipspy:master.

@cdeil
Copy link
Contributor Author

cdeil commented Jul 24, 2017

I am working towards correct HEALPix <-> HiPS tile pixel mapping, so that we can also generate HiPS, not just read and draw it. What I found is that the up-down flip between FITS relative to {PNG/JPEG} is there both for tiles and all-sky images:
https://github.com/cdeil/hipsperiments/tree/master/tile_orientation

One can see that all_sky.tile(ipix) doesn't return the same tile for FITS and JPEG, i.e. it's currently broken for one of the two, and I'm not even sure which one.

I will implement a function that implements HEALPIX ipix to Hips tile (x, y) pixel coordinate mapping next, and that is yet another case where the up / down flip needs to be taken into account.

My current thinking is to move the up / down flip from

def tile_corner_pixel_coordinates(width, file_format) -> np.ndarray:

to a central location where it will be applied correctly for all cases where it needs to be applied.
Maybe it could be part of HipsTile.data (and the corresponding reverse from_numpy) that at that point we call numpy.flipup on the data array?
Which tile orientation should we make the "standard" one in our package? I.e. should we always convert to the FITS orientation or the JPEG orientation?

@adl1995 @tboch - Please comment if you have any thoughts on this.

If I don't hear back, I'll just go ahead and implement a solution I like in this PR and we can then still discuss and change if needed in the coming days / weeks.
I'm leaning towards always converting to the FITS orientation internally, and to always do it at the point where we to to and from numpy array with the data.

@adl1995
Copy link
Member

adl1995 commented Jul 24, 2017

@cdeil I would also prefer if we convert to FITS orientation by default.

@cdeil cdeil changed the title Improve all-sky image implementation and tests Improve tile and allsky orientation handling Jul 25, 2017
@cdeil
Copy link
Contributor Author

cdeil commented Jul 25, 2017

I've made the change to flipping JPG and PNG tiles and all-sky images to the FITS convention in to_numpy and from_numpy in 26b98de in this PR.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.947% when pulling 26b98de on cdeil:allsky2 into 1b36234 on hipspy:master.

@cdeil
Copy link
Contributor Author

cdeil commented Jul 25, 2017

I was really struggling with the orientation of tiles and the whole image for the all-sky images.
I think with acd58c8 now results are correct.

I'm merging this now and will continue with HEALPIX ipix <-> tile (x_idx, y_idx) mapping in a separate future PR.

@cdeil cdeil merged commit a357718 into hipspy:master Jul 25, 2017
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