Add HipsTile and HipsTileMeta classes

[adl1995]

This pull request adds a class called HiPS which provides methods to handle the HiPS base url and tile url.

[coveralls]

Coverage decreased (-3.9%) to 96.104% when pulling bc8adef356f36ea9c2324a7348ab52ef76f5c773 on adl1995:tiles.hips into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

[cdeil]

Just Hips is not a good name. It's too generic, everything in this package is related to Hips.

The functionality you have here now is basically a function that takes three parameters and builds a URL string:

hips.get_tile_url(order=6, ipix=24185, format='jpg') == 'http://alasky.unistra.fr/DSS/DSSColor/Norder6/Dir20000/Npix24185.jpg'

@adl1995 - I'm not sure how best to proceed with this PR.

You could either change the class you have here into a simple function make_hips_tile_url (or some similar name) and also pass in base_url there.

Or you could extend the PR and implement some of the things we discussed via email and in Google hangouts in the past 2 weeks, e.g. a class to represent a tile, and then leave this get_tile_url as a method on that class.

One more comment: eventually we will also need to read / write tiles from disk. There you should use pathlib.Path objects instead of just strings, because the folder separator is \ on Windows and not / like you have here. So in the end, there will be at least two very much related functions or methods or properties that create a URL or filename (and for the filename case there is no "base url", but possible a "base path". I'm OK with refactoring whatever you add here later, unless you already have an idea how to structure the code to support these two use cases.

[adl1995]

@cdeil - I have modified the class name to HipsTile.

I also added some extra methods. Should this class also handle a tile header? If so, a tile header would only be available if the format is 'FITS', right? In other cases, should I just set the variables to None? I was also thinking of adding additional methods such as constructing a WCS object from a HiPS tile, but then again this would only be possible if the tile format is 'FITS'.

Would it be appropriate if I merged the fetching part within this class or should I add a separate class for that?

I'm also getting this error when I try and run the tests:

hips/tiles/tile.py:45: in data
    self.data = data
hips/tiles/tile.py:45: in data
    self.data = data
!!! Recursion detected (same locals & position)

But there is no recursion involved! The issue came when I modified the __init__ method to set the values. I haven't found a solution except for this issue but I have not been able to solve this yet.

[cdeil]

@adl1995 - I think you can just pass data=None in __init__ and set is as self.data = data there and remove the property getter / setter you have for data.

So if this class represents a tile, the most important functionality is that you're able to read a tile from local disk, or from a URL. E.g. you could start with this tile and try to get it to work in your test: https://github.com/hipspy/hips-extra/raw/master/datasets/samples/DSS2Red/Norder3/Dir0/Npix448.fits

First, try to get the reading of the pixel values to work (and e.g. assert on the pixel data[10, 20] value in the test). Tile metadata handling will be a topic, but to just draw tiles, what you need is the pixel values, no?

[cdeil]

One more comment: I'd suggest to use here (and always) the pattern where __init__ just takes the data members and stores them away on self, and things like file I/O or fetching tiles are done in classmethods like read, i.e. the pattern we already used for the one class we currently have: https://hips.readthedocs.io/en/latest/api/hips.HipsDescription.html#hips.HipsDescription.read

[adl1995]

@cdeil I made a commit which adds some extra methods for reading tiles (from disk and URL). It also provides a method for storing a HiPS tile on disk. The default read / write path is hips/tiles/tests/data. These are all standard class methods. As I wanted access to instance variables, I could not use @classmethd. The docstring is updated to contain an example showing fetching of a HiPS tile.

I'm still not able to test the fetch method as it tries to access data from a remote url. Is there any workaround for this?

[adl1995]

@cdeil I added the @remote_data decorator after importing it using: from astropy.tests.helper import remote_data, but I think this just skips the test case. I tested this by adding a line asset 1 == 2 and this did not raise any error.

[cdeil]

Did you try

Turn on the remote data tests at the command line with python setup.py test --remote-data=any. as suggested here? http://docs.astropy.org/en/stable/development/testguide.html#working-with-data-files

[adl1995]

@cdeil - I just made a commit which addresses the issues you mentioned above.

[cdeil]

@adl1995 - Thinking about the HipsTile class a bit more and looking at the code, I noticed that there's some things that aren't nice, probably indicating that we haven't found good abstractions yet / should introduce extra classes.

My first point is that in HipsTile, you store a HipsDescription (require it in __init__), but then never access it, i.e. don't use it for anything.

Note that HipsDescription is not the "meta" or "header" information for a single tile, even if it looks a bit that way. It is the meta or header information for a whole Hips survey, usually consisting of 1000s of tiles at different orders / sky positions.

I think for now HipsDescription should simply be removed from the HipsTile class, and maybe later we introduce a class separate out the "meta" information (like order / ipix) from HipsTile into a HipsTileMeta class. And in addition, I would suggest you rename HipsDescription to HipsSurveyProperties (or HipsSurveyMeta or ...) and update the docstring so that it's clear that this is for a given survey, this is not the metadata for a single tile. Also, adding and example to the HipsDescription docstring that shows how to fetch one properties.txt file and links to it from the docstring (so that the user can quickly look at the example) would be helpful. @adl1995 - Can you make this change for HipsDescription please? Probably best in a separate PR.

---

Other than that, we'll have to discuss more closely how read / write / fetch exactly works and which info for that should be HipsTile class data members or not. At the moment it's a pretty wild mix, with base_url and format being data members, but e.g. base_path for the filename not. Intuitively I would think that the HipsTile class should be minimal and then methods like fetch or read become classmethods that take a base_url and filename as input (and write takes the full filename as output. Would that work, or would we need to introduce a second class to generate urls and filenames then?

[adl1995]

@cdeil - My latest commit removes most of the functionality from the class. I will move the previous method from the class to utils.py when this PR gets merged, otherwise I think there will be Git conflicts. For now, there is a @property in this class, but I'm thinking of moving it to utils.py as well under the name base_path, where all the HiPS tiles will be read / written to.

[adl1995]

@cdeil - I've introduced a new class HipsTileMeta and remove tiles from /data directory. I also added a utility function for getting the default path.

[cdeil]

remove tiles from /data directory.

Note that you still have it in previous commits in this branch. I.e. there's a commit adding the files and then one removing the files. So everyone that download this code repo in the future will have to download those files.

To really get rid of the files, i.e. not introduce them into the repo, I would suggest you squash all the commits here into one. I usually use git rebase -i master to rebase against the latest master, but there's several ways to do this with git.

[coveralls]

Coverage decreased (-16.9%) to 80.0% when pulling 51c1da87203222d355dbb18c1b9fad1140ccae84 on adl1995:tiles.hips into 86284662570e94c0610729c2bf9178e19b74ee09 on hipspy:master.

[adl1995]

@cdeil - I have removed utils/tile.py and introduced some properties in HipsTileMeta. Please check.

[coveralls]

Coverage decreased (-15.5%) to 81.319% when pulling 40b379041ab6aa0dac29fa86832a99cb6149807c on adl1995:tiles.hips into 86284662570e94c0610729c2bf9178e19b74ee09 on hipspy:master.

[coveralls]

Coverage decreased (-15.5%) to 81.319% when pulling 5db3ef2ae3c3958ae86906e840c4bc4793845adf on adl1995:tiles.hips into 86284662570e94c0610729c2bf9178e19b74ee09 on hipspy:master.

[coveralls]

Coverage decreased (-16.6%) to 80.214% when pulling 1136490773612a0601b06b5c8a644d728f0f659b on adl1995:tiles.hips into 86284662570e94c0610729c2bf9178e19b74ee09 on hipspy:master.

[adl1995]

@cdeil - I have changed the type annotation issue and passed in the full file path as the parameter. But, I'm still getting this error:

hips/tiles/tile.py:89: error: Incompatible types in assignment (expression has type "str", variable has type "Path")

The variable path is only populated within the if / else block, so I'm not sure how this is an error.

[coveralls]

Coverage decreased (-15.2%) to 81.667% when pulling 0bdb052115291c0657841c6becf973cec2ff59d6 on adl1995:tiles.hips into 86284662570e94c0610729c2bf9178e19b74ee09 on hipspy:master.

[cdeil]

@adl1995 - See d2ad05b . OK?

[coveralls]

Coverage decreased (-17.06%) to 79.793% when pulling d2ad05b4cbcbe96da194316e61d7e2e9d70af2fd on adl1995:tiles.hips into 86284662570e94c0610729c2bf9178e19b74ee09 on hipspy:master.