hipspy / hips

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

Add class HipsSurveyList for parsing list of hips surveys #27

Closed adl1995 closed 7 years ago

adl1995 commented 7 years ago

I added a class for creating a astropy.table.Table from a text file. It reads a list of OrderedDict and creates a table from it.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.1%) to 98.889% when pulling e58e7b00b634242a2a09e74e237500e8ec2a6491 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

adl1995 commented 7 years ago

@cdeil - I've made some changes. But, the parse method still duplicates the code from HipsDescription. Is there any way I can call the parse @classmethod from within HipsTile?

cdeil commented 7 years ago

My understanding is that the HipsSurveyList created here is a list of https://hips.readthedocs.io/en/latest/api/hips.HipsDescription.html and the text file format for HipsSurveyList is a bunch of text blocks matching the text file format for a single HipsDescription concatenated, separated by an empty line.

Is this correct or not?

If yes, you should get the text blocks by splitting on empty lines as you do, and then call HipsDescription.parse on each of these text blocks. OK?

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.1%) to 97.895% when pulling 673b5cdf9750d2c716354f612f838943ee021659 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

adl1995 commented 7 years ago

@cdeil - I have renamed the class HipsDescription and HipsSurveyList to HipsSurveyList and HipsSurveyPropertyList. I've also updated the parse method to use HipsSurveyList.parse. Please check.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.3%) to 97.674% when pulling f91a6999772f2271e8a15151694d63d003807594 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.3%) to 97.674% when pulling 2fbf0cbb371c16fc8603d7d17b976b857d1de579 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.3%) to 97.701% when pulling 96c9b840abf37440b49cc4469b75ef25fbb7dbae on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.3%) to 97.701% when pulling 6e086de91e8baecb31ae015d22d5c166eafb8a98 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

adl1995 commented 7 years ago

@cdeil - I have modified the type to List[HipsSurveyProperties] in the __init__ method, but this gives me an error when running mypy:

hips/tiles/surveys.py:61: error: Argument 1 to "HipsSurveyPropertiesList" has incompatible type List[OrderedDict[Any, Any]]; expected List[HipsSurveyProperties]

So, I haven't pushed my changes yet. I don't have much experience with mypy, but I tried changing all the type annotations from HipsSurveyPropertiesList to List[HipsSurveyProperties]. This removes all the errors expect for this one:

hips/tiles/surveys.py:61: error: Incompatible return value type (got "HipsSurveyPropertiesList", expected List[HipsSurveyProperties])

hips/tiles/surveys.py:61: error: Argument 1 to "HipsSurveyPropertiesList" has incompatible type List[OrderedDict[Any, Any]]; expected List[HipsSurveyProperties]

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.2%) to 97.778% when pulling 6e086de91e8baecb31ae015d22d5c166eafb8a98 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

adl1995 commented 7 years ago

@cdeil - I tried response.decode('utf-8'), but this gives me an error:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xb2 in position 83958: invalid start byte

Also, I think you meant to remove the .properties from HipsSurveyProperties, this removes the mypy error, but I'm unable to use Astropy Table with this.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.4%) to 96.552% when pulling 511395862ac03f22501b4dc738a687c8ea890189 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.3%) to 96.739% when pulling 46b9de447eb596e49ec56b4bc98187d5e73713be on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.3%) to 96.739% when pulling c7345f005622ad54b9bef7fe439536185e1b6218 on adl1995:hips.surveys into ec1cfae57a28f1443664a3ee64232ff3d83c4645 on hipspy:master.

adl1995 commented 7 years ago

@cdeil - I added the fetch classmethod, but also re-based it against master, so there are also other files changes showing up.

cdeil commented 7 years ago

@adl1995 - I think for this PR you also rebased against some old master branch, so now there's a lot of stuff in the diff unrelated to this PR.

Please rebase against latest master and squash all commits into a single one with a good commit message.

cdeil commented 7 years ago

@adl1995 - I've added a commit in c12acd9ab7797cd7fd7e08a80f0ee802715e95f3 that fixes HipsSurveyPropertiesList.fetch (conversion to text and parsing was broken) and adds a test for that, adds from .surveys import * to __init__ so that the class shows up in the docs, as well as a bunch of cleanup. I did rename the data members on the HipsSurveyProperties and HipsSurveyPropertiesList classes to just "data", because I found the previous names confusing because those were often the names of the local variable (i.e. survey.survey or properties.properties is confusing).

@adl1995 - Will merge soon when travis-ci passes, unless you have any comments.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 80.426% when pulling c12acd9ab7797cd7fd7e08a80f0ee802715e95f3 on adl1995:hips.surveys into b00cef082b147583754c9a534d7c098a866033d4 on hipspy:master.