Add option to create HipsSurveyProperties from survey name #95

adl1995 · 2017-07-27T13:22:34Z

As outlined in #82 and during the telecon yesterday, I have added the option to create HipsSurveyProperties using a survey name.

@cdeil Currently, this is not implemented:

In addition, we might want to allow the geometry parameter in make_sky_image to be a dict, and if it is, call WCSGeometry.create_from_dict() for the user as a convenience. I.e. this would work:
geometry = dict(target='crab', fov='10 deg', frame='galactic')
make_sky_image(geometry, hips_survey)

Can this be part of a separate PR?

coveralls · 2017-07-27T13:52:34Z

Coverage increased (+0.5%) to 96.844% when pulling f6a63fa on adl1995:WCSGeometry.from_name into 9cef150 on hipspy:master.

cdeil

I left two inline comments.

Note: https://travis-ci.org/hipspy/hips/jobs/258142624#L1545
Do you know hot to fix those or should I explain?

Yes, the similar change to geometry should definitely be a separate PR.

cdeil · 2017-07-27T14:34:16Z

docs/getting_started.rst

-    url = 'http://alasky.unistra.fr/DSS/DSS2Merged/properties'
-    hips_survey = HipsSurveyProperties.fetch(url)
-
+    hips_survey = HipsSurveyProperties.make('CDS/P/DSS2/red')


If you remove the import and just write this here

hips_survey = 'CDS/P/DSS2/red'

it would give the same result, no?

Please make it work that way and change the high-level docs to use that simple form.

cdeil · 2017-07-27T14:35:34Z

hips/draw/simple.py

@@ -61,7 +61,7 @@ class HipsPainter:
    (1000, 2000)
    """

-    def __init__(self, geometry: WCSGeometry, hips_survey: HipsSurveyProperties, tile_format: str) -> None:
+    def __init__(self, geometry: WCSGeometry, hips_survey: str or HipsSurveyProperties, tile_format: str) -> None:


I think the type annotation has to be Union in case of multiple options, no?
https://docs.python.org/3/library/typing.html#typing.Union

adl1995 · 2017-07-27T16:10:40Z

@cdeil I've made the changes. I also marked the tests with remote_data. Hopefully, the build will pass now.

cdeil

I've left a few inline comments.
When those are adressed, this PR can go in.
It's an important user experience improvement, so thanks for putting it together quickly, @adl1995 !

cdeil · 2017-07-27T16:43:24Z

docs/getting_started.rst

@@ -26,21 +26,16 @@ To draw a sky image from HiPS image tiles with the `hips` package, follow the fo
         coordsys='galactic', projection='AIT',
    )

-
-2. Specify the HiPS survey you want by creating a `~hips.HipsSurveyProperties` object.
+2. Specify the HiPS survey you want.


You should mention that users have to give the ID of the Hips survey.

cdeil · 2017-07-27T16:44:14Z

hips/draw/simple.py

@@ -35,7 +35,7 @@ class HipsPainter:
    ----------
    geometry : `~hips.utils.WCSGeometry`
        An object of WCSGeometry
-    hips_survey : str or `~hips.HipsSurveyProperties`
+    hips_survey : `str` or `~hips.HipsSurveyProperties`


I'm using the docs pattern where I don't put backticks for PYthon built-in types like str, list, int, ...
So please remove them here for consistency with the rest of our docstrings.

cdeil · 2017-07-27T16:45:27Z

hips/draw/simple.py

@@ -61,7 +61,7 @@ class HipsPainter:
    (1000, 2000)
    """

-    def __init__(self, geometry: WCSGeometry, hips_survey: HipsSurveyProperties, tile_format: str) -> None:
+    def __init__(self, geometry: WCSGeometry, hips_survey: Union[str, 'HipsSurveyProperties'], tile_format: str) -> None:


The common thing to do is to put the type, and only to put quotes if there's a forward reference issue.
So please remove the quotes from HipsSurveyProperties here in the type annotation, and review the diff of this PR on Github to fix the other cases (I see one below).

cdeil · 2017-07-27T16:46:40Z

hips/draw/simple.py

-    image : `~numpy.ndarray`
-        Output image pixels
+    result : `~hips.HipsDrawResult`
+        HipsDrawResult object


Don't duplicate the type name in the description. It's not useful. Here I'd suggest this desription:

Result object

cdeil · 2017-07-27T16:55:54Z

hips/tiles/tests/test_surveys.py

-        # TODO: look up survey by name here
-        # Otherwise this will break when the list changes
-        survey = surveys.data[0]
+        survey = HipsSurveyProperties.from_name('CDS/P/2MASS/H')


This change to the test is no good.

I would suggest to move the code that does the name lookup from the HipsSurveyProperties.from_name method to a HipsSurveyPropertiesList.from_name method and then call this here from this test (instead of calling

HipsSurveyProperties.from_name('CDS/P/2MASS/H')

which will fetch the list again.

The implementation of HipsSurveyProperties.from_name would then just be

@classmethod def from_name(name); surveys = HipsSurveyPropertiesList.fetch() return surveys.from_name(name)

and maybe we'd remove it eventually.

You should also add a test with an invalid key like name='kronka lonka' and check that a KeyError is raised.
I explained how to do that with pytest.raises here previously: #55 (comment)

@cdeil I'm not sure what you mean here. Do you mean to say create a simple class method from_name in addition to the classmethod?

I've updated my comment above: the class names I gave were wrong. I meant adding a normal method HipsSurveyPropertiesList.from_name (not a classmethod), because really looking for a name is an operation on the list, i.e. the code that does the looping through surveys and looking for a name ID should be a method on HipsSurveyPropertiesList.

Is it clear now?

cdeil · 2017-07-27T16:57:46Z

hips/tiles/tests/test_surveys.py

+    def test_make(self):
+        survey = HipsSurveyProperties.make('CDS/P/EGRET/Dif/300-500')
+        assert survey.title == 'EGRET Dif 300-500'
+        self.survey = HipsSurveyProperties.make(self.survey)


This line doesn't make sense. You're setting self.survey on the last line of the test.
Instead you should be asserting something.
I think this might work:

assert self.survey is HipsSurveyProperties.make(self.survey)

because in that case we just return the same object and "is" checks for object equality, no?

coveralls · 2017-07-27T17:10:01Z

Coverage increased (+0.5%) to 96.844% when pulling b84b776 on adl1995:WCSGeometry.from_name into 9cef150 on hipspy:master.

cdeil · 2017-07-28T07:10:06Z

Thanks!

coveralls · 2017-07-28T07:18:47Z

Coverage increased (+0.7%) to 97.02% when pulling 09906ec on adl1995:WCSGeometry.from_name into 9cef150 on hipspy:master.

cdeil added this to the 0.1 milestone Jul 27, 2017

cdeil self-assigned this Jul 27, 2017

cdeil added the enhancement label Jul 27, 2017

cdeil suggested changes Jul 27, 2017

View reviewed changes

adl1995 force-pushed the WCSGeometry.from_name branch 2 times, most recently from bd13465 to b84b776 Compare July 27, 2017 16:30

cdeil suggested changes Jul 27, 2017

View reviewed changes

Add option to create HipsSurveyProperties from survey name

09906ec

adl1995 force-pushed the WCSGeometry.from_name branch from b84b776 to 09906ec Compare July 28, 2017 06:46

cdeil merged commit e9b1dfa into hipspy:master Jul 28, 2017

cdeil mentioned this pull request Aug 23, 2017

Introduce a simple way to select a HiPS by ID string #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to create HipsSurveyProperties from survey name #95

Add option to create HipsSurveyProperties from survey name #95

adl1995 commented Jul 27, 2017 •

edited

Loading

coveralls commented Jul 27, 2017

cdeil left a comment

cdeil Jul 27, 2017

cdeil Jul 27, 2017

adl1995 commented Jul 27, 2017

cdeil left a comment

cdeil Jul 27, 2017

cdeil Jul 27, 2017

cdeil Jul 27, 2017

cdeil Jul 27, 2017

cdeil Jul 27, 2017 •

edited

Loading

adl1995 Jul 27, 2017

cdeil Jul 27, 2017

cdeil Jul 27, 2017

coveralls commented Jul 27, 2017

cdeil commented Jul 28, 2017

coveralls commented Jul 28, 2017

Add option to create HipsSurveyProperties from survey name #95

Add option to create HipsSurveyProperties from survey name #95

Conversation

adl1995 commented Jul 27, 2017 • edited Loading

coveralls commented Jul 27, 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

adl1995 commented Jul 27, 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

cdeil Jul 27, 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

coveralls commented Jul 27, 2017

cdeil commented Jul 28, 2017

coveralls commented Jul 28, 2017

adl1995 commented Jul 27, 2017 •

edited

Loading

cdeil Jul 27, 2017 •

edited

Loading