Add classmethod create_simple to WCSGeometry #49

adl1995 · 2017-07-03T10:12:49Z

Added a classmethod as mentioned in #39.

cdeil

@adl1995 - I've left some inline comments.

When writing the tests for this method, you should especially add asserts for the things you compute, i.e. cdelt and crpix.

cdeil · 2017-07-03T10:16:54Z

hips/utils/wcs.py

+        projection : `str`
+            Projection of the WCS object
+        """
+


Remove empty line.

cdeil · 2017-07-03T10:17:58Z

hips/utils/wcs.py

@@ -124,6 +124,30 @@ def create(cls, skydir: SkyCoord, shape: tuple, coordsys: str = 'CEL',

        return cls(w, shape)

+    @classmethod
+    def create_simple(cls, skydir: SkyCoord, shape: tuple, fov_deg: float,
+                      coordsys: str = 'CEL', projection: str = 'AIT') -> 'WCSGeometry':


For coordsys, I would suggest we change this method and create to support {'icrs', 'galactic'}.
Having yet another code in the end-user API for the RADEC frame ("CEL" at the moment) is confusing.

Do you mean to pass 'icrs' and 'galactic' instead of 'CEL' and 'GAL'?

Yes.

I would suggest the end-user API of the hips package always has "icrs" and "galactic", and where needed we convert to other strings under the hood (e.g. to compare with hips_frame).

cdeil · 2017-07-03T10:19:19Z

hips/utils/wcs.py

+            Field of view in degrees
+        coordsys : `str`
+            Coordinate system
+        projection : `str`


You should link to http://docs.astropy.org/en/stable/wcs/#supported-projections so that users know what choices are available.

cdeil · 2017-07-03T10:21:31Z

hips/utils/wcs.py

+            Sky coordinate of the WCS reference point
+        shape : `tuple`
+            Shape of the image (Numpy axis order: y, x)
+        fov_deg : `float`


I would suggest to change this option to fov and then in the code do this:

from astropy.coordinates import Angle fov = Angle(fov)

This will force the user to be explicit about units of inputs (lead to more readable user scripts) and allow e.g. to pass strings like "3 deg" (i.e. users don't have to instantiate an Angle object before calling.

cdeil · 2017-07-03T10:23:56Z

hips/utils/testing.py

@@ -58,5 +57,17 @@ def make_test_wcs_geometry(case=0):
            shape=(1000, 2000), coordsys='GAL',
            projection='AIT', cdelt=0.01, crpix=(1000, 500),
        )
+    elif case == 3:


I suggest you move these cases simply to unit tests of WCSGeometry in TestWCSGeometry.
I.e. have test methods that instantiate a WCSGeometry by calling this method and then assert some properties of the resulting object.

The only reason for this make_test_wcs_geometry was to have something available for other tests.
We should keep the number of cases here as small as possible, i.e. simple unit tests for WCSGeometry go to TestWCSGeometry.

cdeil · 2017-07-03T10:27:28Z

hips/utils/wcs.py

+        coordsys : `str`
+            Coordinate system
+        projection : `str`
+            Projection of the WCS object


Add an Examples section with an example (maybe the same one as you have for create?
I would also suggest that you use this more convenient method here:
https://hips.readthedocs.io/en/latest/getting_started.html#make-a-sky-image

coveralls · 2017-07-03T10:31:07Z

Coverage increased (+1.2%) to 87.39% when pulling 2a4229e on adl1995:create_wcs into 84d60c1 on hipspy:master.

adl1995 · 2017-07-03T11:32:55Z

@cdeil - I just made another commit. Hopefully this resolves all the issues.

cdeil · 2017-07-03T11:48:47Z

See https://travis-ci.org/hipspy/hips/jobs/249595428#L1148
(always run tests locally before pushing)

Before we merge I have two questions on user interface, @tboch and @adl1995 - please comment:

The names create and create_simple aren't great. I don't have a suggestion for better names. Thoughts?
Maybe we should take separate arguments width and height or nx and ny for the number of pixels? (In Add WCSGeometry create_simple that matches Aladin Lite #39 (comment) @tboch commented that he's taking width and height in Aladin Lite). I think if we take a shape, especially with the current order (ny, nx), but even if we flip, users will get it wrong all the time . So here I'm in favour of either width and height or nx and ny instead of shape.

cdeil · 2017-07-03T11:49:55Z

hips/utils/wcs.py

+            Sky coordinate of the WCS reference point
+        shape : `tuple`
+            Shape of the image (Numpy axis order: y, x)
+        fov: `str`


fov can be Angle or str, not just str.

@adl1995 - Please address this one remaining inline comment. Otherwise this is 👍 from my side.

@tboch - Assiging to you for final review.

adl1995 · 2017-07-03T12:46:50Z

@cdeil - How about create_from_fov? I'm fine with taking two separate arguments instead of shape.

adl1995 · 2017-07-04T07:51:20Z

I made another commit. However, as I pulled the latest code from master branch, I now have a commit named:

Merge branch 'master' of https://github.com/hipspy/hips into create_wcs

cdeil · 2017-07-04T07:54:04Z

The common way to avoid these merge commits is to "rebase" instead of "merging".
Please read the Astropy dev docs (or other resources) explaining this and do that in the future.
For this PR: either leave as-is, or fix it up (e.g. by squashing commits, or doing a hard reset on the commit before the merge commit).

adl1995 · 2017-07-04T07:57:33Z

@cdeil - I have removed the merge commit.

tboch · 2017-07-04T08:08:35Z

Things look fine for me.
I like @cdeil 's suggestion to split the shape argument into width and height. Do we leave that for another pull request?

outdated

cdeil · 2017-07-05T12:31:02Z

As discussed in telcon: suggest to keep Shape attribute on WCSGeometry as-is, but only to take parameters nx and ny separately in constructors and make and store the Shape on self.

cdeil · 2017-07-05T12:31:47Z

Correction: call it width and height (both in the constructor arguments, and in the Shape attributes.

coveralls · 2017-07-05T14:39:35Z

Coverage increased (+1.2%) to 86.726% when pulling c5d4615 on adl1995:create_wcs into e6e8747 on hipspy:master.

adl1995 · 2017-07-05T15:16:22Z

@cdeil I have made a commit updating the shape parameter.

coveralls · 2017-07-05T15:29:15Z

Coverage increased (+1.2%) to 86.726% when pulling 1465e55 on adl1995:create_wcs into e6e8747 on hipspy:master.

cdeil

I've left some inline comments.

The most important one is the question whether crpix is filled correctly or whether there's a but where x / y is exchanged when filling it.

cdeil · 2017-07-05T17:56:23Z

docs/getting_started.rst

         skydir=SkyCoord(0, 0, unit='deg', frame='galactic'),
-         shape=(1000, 2000), coordsys='GAL',
-         projection='AIT', cdelt=0.01, crpix=(1000, 500),
+         shape=(1000, 2000), fov="3 deg",


This still calls with shape, needs update, no?

cdeil · 2017-07-05T17:57:05Z

hips/utils/testing.py

@@ -43,19 +42,19 @@ def make_test_wcs_geometry(case=0):
    if case == 0:
        return WCSGeometry.create(
            skydir=SkyCoord(3, 4, unit='deg', frame='galactic'),
-            shape=(2, 3), coordsys='GAL',
+            height=2, width=3, coordsys='galactic',


I'd suggest we always write in the order width, then height, even if it doesn't matter when calling with kwargs. Please change here and below.

cdeil · 2017-07-05T17:58:18Z

hips/utils/wcs.py

    """
    WCS_ORIGIN_DEFAULT = 0
    """Default WCS transform origin, to be used in all WCS pix <-> world calls."""

-    def __init__(self, wcs: WCS, shape: tuple) -> None:
+    def __init__(self, wcs: WCS, height: int, width: int) -> None:


I'd suggest we always use the order: width, then height , i.e. please change this here.

cdeil · 2017-07-05T17:59:10Z

hips/utils/wcs.py

+        height : `int`
+            Height of the image
+        width : `int`
+            Width of the image


Mention "in pixels" or "number of pixel" to make it perfectly clear that this is not width e.g. as an angle on the sky.

cdeil · 2017-07-05T18:01:47Z

hips/utils/tests/test_wcs.py

+        assert c.frame.name == 'icrs'
+        assert_allclose(c.ra.deg, 359.24, atol=1e-2)
+        assert_allclose(c.dec.deg, -0.74, atol=1e-2)
+        assert_allclose(self.wcs_geometry.wcs.wcs.crpix, [500.,  1000.])


Is this correct? Doesn't crpix have order x, y and should be 1000, 500 here?

@adl1995 - You could check by making a test image locally and e.g. filling the data with zeros or with coordinate values like we do somewhere else, and then writing to FITS and opening it up.

@cdeil How exactly can I check this? I created an image using the survey in test_simple.py and loaded the FITS image in Aladin Desktop. It looks okay to me, but is there anything else that I should be checking?

E.g. ds9, but I think also Aladin will show the mouse position in pixel and world coordinates when you hover over the image.

If you go to the reference point of the projection (the crval sky coordinates), what pixel coordinates do you see?
It should be x=1000 and y=500, but I suspect you'll find it switched.

And you can also try to draw a sky image with this geometry like in this high-level docs example we have. Does it come out correctly or are there issues?

I just tried this. I created the WCSGeometry object this way:

geometry = WCSGeometry.create(skydir=SkyCoord(0, 0, unit='deg', frame='galactic'), width=2000, height=1000, coordsys='galactic', projection='AIT', cdelt=0.01, crpix=(500, 1000))

i.e. reversed the order crpix. This is the result I'm getting in Aladin:

This should be reversed, no?

I just tried reversing crpix values i.e.
geometry = WCSGeometry.create(skydir=SkyCoord(0, 0, unit='deg', frame='galactic'), width=2000, height=1000, coordsys='galactic', projection='AIT', cdelt=0.01, crpix=(1000, 500))
But I still get the same result as above.

The test and my comment were about create_simple and how it fills crpix.
Please use that for checking.

It's hard for me to comment here without seeing which code exactly you ran.
Maybe you could make a notebook that has code like the current getting started, but using create_simple and at the end plot the sky image and a marker using crpix and crval to show where it is.
Then I could tell you if there currently is a bug or not (or probalby you'll understand the situation yourself when doing this).

cdeil · 2017-07-05T18:02:45Z

hips/utils/wcs.py

-__doctest_skip__ = ['WCSGeometry']
+__doctest_skip__ = [
+    'WCSGeometry',
+    'WCSGeometry.*'


Why is this second entry WCSGeometry.* needed here?
Shoudn't the first WCSGeometry entry lead to all doctests in this class being skipped?
Please consult the Astropy docs on this point if you're not sure.

This is required to skip doctests for all the methods within WCSGeometry, in this case the create_simple @classmethod, more here.

I find that weird that listing a class doesn't mean skip all doctests (including methods) in that class. But OK, if that's how it works, then keep as is of course.

One alternative would be to use * in place of both WCSGeometry and WCSGeometry.* which would skip all the tests. Do you think that is more suitable?

Well generally, we should probably first try to write docs examples where the docstests pass.
I think you already saw how if some docs example is untested, it will usually break quickly as we change and refactor the code.
So for now, we have to manually go through and review the docs examples from time to time, e.g. before a release.

For now, do as you like here: fix up the docs example so that it works, or skip in whatever way you like (maybe best to make it as narrow as possible, i.e. just list the method with the doctest you can't get to work)?

coveralls · 2017-07-06T14:20:21Z

Coverage increased (+1.2%) to 87.059% when pulling 8cdd34c on adl1995:create_wcs into 4264f53 on hipspy:master.

adl1995 · 2017-07-06T14:23:57Z

@cdeil I have updated the test cases. The build is passing now. However, I did make some changes on my local notebook, but the results looked identical to me. Please verify before merging.

cdeil · 2017-07-07T06:48:21Z

@adl1995 - Thanks for fixing the bug where the values in crpix were switched in create_simple.

I've attached two commits here, one with minor cleanup, and one fixing this test fail:
https://gist.github.com/cdeil/04226e02ea9c924d24ae91d53e75dcf5

For now I've simply updated the reference value, I'm not sure which is correct.
Also, the high-level docs example is broken now (stuck fetching tiles forever).

I think we should just merge the open PRs in today, especially the tile order computation step, and then re-start debugging.

coveralls · 2017-07-07T06:56:34Z

Coverage increased (+1.2%) to 87.059% when pulling 9b02fe3 on adl1995:create_wcs into 4264f53 on hipspy:master.

cdeil previously requested changes Jul 3, 2017

View reviewed changes

cdeil self-assigned this Jul 3, 2017

cdeil added the enhancement label Jul 3, 2017

cdeil added this to the 0.1 milestone Jul 3, 2017

adl1995 force-pushed the create_wcs branch from 2a4229e to 24dc11e Compare July 3, 2017 11:31

cdeil reviewed Jul 3, 2017

View reviewed changes

cdeil assigned tboch and unassigned cdeil Jul 4, 2017

adl1995 force-pushed the create_wcs branch from 24dc11e to b6846b1 Compare July 4, 2017 07:37

adl1995 force-pushed the create_wcs branch from b6846b1 to 55f296b Compare July 4, 2017 07:57

adl1995 force-pushed the create_wcs branch from 55f296b to bcd9e5d Compare July 4, 2017 10:05

adl1995 force-pushed the create_wcs branch 2 times, most recently from f4599dd to c5d4615 Compare July 5, 2017 14:19

adl1995 force-pushed the create_wcs branch from c5d4615 to 1465e55 Compare July 5, 2017 14:54

cdeil suggested changes Jul 5, 2017

View reviewed changes

adl1995 force-pushed the create_wcs branch from 1465e55 to cfb2264 Compare July 6, 2017 12:35

Add classmethod create_simple to WCSGeometry

8cdd34c

adl1995 force-pushed the create_wcs branch from cfb2264 to 8cdd34c Compare July 6, 2017 14:03

adl1995 mentioned this pull request Jul 7, 2017

Add function for computing HiPS order #58

Merged

cdeil assigned cdeil and unassigned tboch Jul 7, 2017

cdeil approved these changes Jul 7, 2017

View reviewed changes

cdeil added 2 commits July 7, 2017 08:40

Small cleanup of WCSGeometry.create_simple

e9f057f

Update ref value in hips/draw/tests/test_simple.py

9b02fe3

cdeil merged commit 39187d0 into hipspy:master Jul 7, 2017

Add classmethod create_simple to WCSGeometry #49

Add classmethod create_simple to WCSGeometry #49

Conversation

adl1995 commented Jul 3, 2017

cdeil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 3, 2017

adl1995 commented Jul 3, 2017

cdeil commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adl1995 commented Jul 3, 2017

adl1995 commented Jul 4, 2017

cdeil commented Jul 4, 2017

adl1995 commented Jul 4, 2017

tboch commented Jul 4, 2017

cdeil commented Jul 5, 2017

cdeil commented Jul 5, 2017

coveralls commented Jul 5, 2017

adl1995 commented Jul 5, 2017

coveralls commented Jul 5, 2017

cdeil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adl1995 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

adl1995 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 6, 2017

adl1995 commented Jul 6, 2017

cdeil commented Jul 7, 2017

coveralls commented Jul 7, 2017

adl1995 Jul 6, 2017 •

edited

Loading

adl1995 Jul 6, 2017 •

edited

Loading