Add Dastcom5 Service #115

shreyasbapat · 2019-02-27T15:41:26Z

As discussed in chats of astroquery: https://astropy.slack.com/archives/C8U8VGQFM/p1549754624034000

And from a PR in astroquery: astropy/astroquery#1339

I am adding this module here.

cc @Juanlu001

pep8speaks · 2019-02-27T15:41:30Z

Hello @shreyasbapat! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file sbpy/data/utils/dastcom5.py:

Line 355:32: E231 missing whitespace after ','
Line 356:29: E231 missing whitespace after ','
Line 357:31: E231 missing whitespace after ','
Line 358:31: E231 missing whitespace after ','
Line 359:32: E231 missing whitespace after ','
Line 360:32: E231 missing whitespace after ','
Line 361:29: E231 missing whitespace after ','
Line 362:26: E231 missing whitespace after ','
Line 366:1: E302 expected 2 blank lines, found 1

Comment last updated at 2020-07-22 20:39:45 UTC

astrojuanlu · 2019-02-27T15:47:44Z

(In case the Slack message is lost: https://matrix.to/#/!DdazxkLDTPnBLekbbL:openastronomy.org/$15497546251785GFeBH:openastronomy.org?via=cadair.com&via=openastronomy.org&via=matrix.org)

astrojuanlu · 2019-02-28T09:38:09Z

@shreyasbapat please fix the PEP8 errors.

shreyasbapat · 2019-02-28T10:23:19Z

@Juanlu001 I am also working on tests. Will push both of them together

mommermi · 2019-02-28T16:55:13Z

Thanks for providing this PR! A few things that I would like to see discussed/changed/added before I proceed with an in-depth review:

sbpy.data.Orbit handles orbits, so that would be a natural place for dastcom5. However, given the rather complex structure of the code makes it hard to integrate into the existing Orbit class. The current convention is to only have high-level functionality (basically wrappers) in Orbit, such as Orbit.from_horizons or Orbit.from_mpc. Would it be possible to add the current dastcom5 submodule as sbpy.data.utils.dastcom5 and then provide a Orbit.from_dastcom5 wrapper?
The output of the wrapper function should be an Orbit object, which allows it to be used in other sbpy functionality. Functions to convert a astropy.table.Table to Orbit exists in the parent class DataClass.from_table.
we try to avoid pandas and use astropy.table functionality instead. It seems to me that pandas is only used to concatenate tables and order data. This can easily be done on the astropy.table.Table objects or even the resulting Orbit object.
Could you also please add some examples to the docstrings and write a bit of text for docs/sbpy/data.rst?

Thanks!

shreyasbapat · 2019-02-28T19:05:59Z

Sure @mommermi . Thanks for a quick response. I will keep you updated as I complete all the requirements.

shreyasbapat · 2019-03-08T23:05:06Z

I have created the checklist for reference.

sbpy.data.Orbit handles orbits, so that would be a natural place for dastcom5. However, given the rather complex structure of the code makes it hard to integrate into the existing Orbit class. The current convention is to only have high-level functionality (basically wrappers) in Orbit, such as Orbit.from_horizons or Orbit.from_mpc. Would it be possible to add the current dastcom5 submodule as sbpy.data.utils.dastcom5 and then provide a Orbit.from_dastcom5 wrapper?
The output of the wrapper function should be an Orbit object, which allows it to be used in other sbpy functionality. Functions to convert a astropy.table.Table to Orbit exists in the parent class DataClass.from_table.
we try to avoid pandas and use astropy.table functionality instead. It seems to me that pandas is only used to concatenate tables and order data. This can easily be done on the astropy.table.Table objects or even the resulting Orbit object.
Could you also please add some examples to the docstrings and write a bit of text for docs/sbpy/data.rst?

shreyasbapat · 2019-03-13T11:20:05Z

@mommermi I have done everything that was asked!

mommermi · 2019-03-18T17:22:21Z

Thanks, @shreyasbapat ! I will work on the review as soon as possible!

mommermi

Thanks for working on this! I have a handful of comments and suggestions to make this compatible with the sbpy API:

please move the module from sbpy.utils to sbpy.data.utils
please add some examples to the docstrings of the individual functions and methods
in their current state, the return values of most functions are not very useful and not compatible with other sbpy functionality; please make sure that each function returns some derivative of sbpy.data.DataClass where the data is split into columns, each of which uses the corresponding astropy.units unit (see detailed review comments and the wiki: https://github.com/NASA-Planetary-Science/sbpy/wiki)

sbpy/utils/dastcom5_README.txt

mommermi · 2019-03-28T17:37:47Z

sbpy/utils/dastcom5.py

+    return body_data
+
+
+def download_dastcom5():


I like the download function, but I would suggest a few minor things:

during download and in addition to showing the download progress, could you add a line saying: 'downloading current DASTCOM5 database' or something similar?

can you add a keyword parameter update that allows to overwrite the local version of the database?

instead of downloading the files into ~/.sbpy, can you download them into data_archive in the sbpy directory?

in order to save space, please delete the zip file once it is unpacked

update will mean removing the version user has and downloading completely newer version? I didn't understand.

Yes, sorry if I was unclear. By 'update' I mean to overwrite the existing local version with the latest available version.

instead of downloading the files into ~/.sbpy, can you download them into data_archive in the sbpy directory

Won't the users installing it from pip find it difficult to access the database? I think it is fine if someone is cloning the repository somewhere and then runs pip install -e .. Otherwise in my opinion, it won't be good?

Good point. I don't think we should assume that the sbpy directory is writable.

Ok, so will we use ~/.sbpy as the location of the database? ~/sbpy/dastcom5/?

Yes @mommermi
/.sbpy/dastcom5/

mommermi · 2019-03-28T17:39:02Z

sbpy/utils/dastcom5.py

+        Database with custom dtype.
+
+    """
+    with open(AST_DB_PATH, "rb") as f:


It seems that there is some kind of timestamp in the database; can you issue a warning if the local database is older than 7 days? The same applies to the comet database.

Where do you see the timestamp? I am unable to find it :/

I might have mistaken Epoch as a timestamp. Should we use this?

I don't think using epoch is a good idea for giving out warnings.

sbpy/data/orbit.py

mommermi · 2019-03-28T17:59:52Z

sbpy/utils/dastcom5.py

+    return data
+
+
+def orbit_from_name(name):


Both this function and orbit_from_record lack a parser function. Right now, all the data is stored in a single column of an astropy.table object, which is not very useful. Please split up the data into columns in a useful way; have a look at https://sbpy.readthedocs.io/en/latest/fieldnames.html and the documentation of astroquery.jplhorizons/astroquery.sbdb to get an idea of meaningful column names for the different properties that are already being used within sbpy. Finally, please use the corresponding astropy.units for each column.

Can you please elaborate a little by what you mean by parser? Sorry that I am unable to understand. Will it be on the top of these pre-existing functions and return the required table?

Yes, exactly. Right now, the functions return single-columns tables. These tables are not really useful as the data are unstructured. A parser function would read in these rows and split them into the individual columns. Does this make sense?

Should this function still return a table?

Added. Please review what name it should have. I have added a function with a wierd name :/

mommermi · 2019-03-28T18:00:30Z

sbpy/utils/dastcom5.py

+    return records
+
+
+def string_record_from_name(name):


this function should be mentioned in the documentation

mommermi · 2019-03-28T18:01:25Z

sbpy/utils/dastcom5.py

+    return lines
+
+
+def read_headers():


what does this function do? What are 'headers'?

The HEADERS of the database

mommermi · 2019-03-28T18:04:26Z

sbpy/utils/dastcom5.py

+        for line in inF:
+            if re.search(r"\b" + name.casefold() + r"\b", line.casefold()):
+                lines.append(line)
+    return lines


please turn this output into a sbpy.data.DataClass object with corresponding column names.

mommermi · 2019-03-28T18:09:18Z

sbpy/utils/dastcom5.py

+    return ast_header, com_header
+
+
+def read_record(record):


I couldn't get this function to work. It fails with

File ".../sbpy/utils/dastcom5.py", line 502, in read_record
f.seek(phis_rec, os.SEEK_SET)

This function would be potentially useful and could output as sbpy.data.Phys object.

Will work on it

This function is already being used in orbit_from_record 😅

This error pops when the record is not there in the database. Please try with :

read_record(1636)

which is for atira

shreyasbapat · 2019-05-05T21:16:40Z

@mommermi I am facing issues in pip installing the package.

Failed building wheel for synphot
  Running setup.py clean for synphot
Failed to build synphot

Is this my environment problem?
Also, I seem to not find a place to report this bug

astrojuanlu · 2019-05-05T22:22:22Z

Failed to build synphot

I just did pip install git+https://github.com/NASA-Planetary-Science/sbpy.git on Linux + Python 3.7 and worked just fine.

astrojuanlu · 2019-05-05T22:23:51Z

Also, I seem to not find a place to report this bug

https://github.com/spacetelescope/synphot_refactor

mommermi · 2019-05-06T15:38:51Z

@shreyasbapat: have you tried installing synphot separately with: pip install synphot?

shreyasbapat · 2019-06-20T13:26:31Z

I am sorry for leaving this for long! I will finish this up asap!

shreyasbapat · 2020-01-04T11:20:30Z

Finally, a good rebase. I will finish the PR in a day.

.travis.yml

mkelley · 2020-02-24T14:27:27Z

@shreyasbapat @Juanlu001 The test needs to be skipped or marked as remote, probably the latter? Append "# doctest: +REMOTE_DATA" to the lines that need remote data. https://docs.astropy.org/en/stable/development/testguide.html#working-with-data-files

shreyasbapat · 2020-02-27T17:37:04Z

Sure. I will do so.

shreyasbapat · 2020-03-31T05:46:35Z

@mkelley The # doctest: +REMOTE_DATA is for usage in docs right? What if I had a test which is to test if my download happens, can I skip the whole test in Travis?

mommermi · 2020-03-31T06:49:29Z

If you have a doctest that checks if your download succeeded, I think it's legitimate to skip it. However, in that case, you should have a dedicated test on the module level that checks for this.

mkelley · 2020-04-08T17:29:04Z

@shreyasbapat @mommermi I don't think the download should ever happen in the documentation. The files are too large. If there were a way to do a mock download, that would be OK.

shreyasbapat · 2020-04-08T17:45:38Z

True. I will try to get the ticks green this weekend.

astrojuanlu · 2020-04-15T12:18:44Z

In case it's relevant for this PR, we fixed a bug in the original code poliastro/poliastro#902

shreyasbapat · 2020-04-15T13:31:23Z

Will fix that asap here also.

…

_______________________________________ This email is sent from a smartphone. Please ignore grammatical mistakes and typographical errors.

_______________________________________

On Wed, Apr 15, 2020, 5:48 PM Juan Luis Cano Rodríguez < ***@***.***> wrote: In case it's relevant for this PR, we fixed a bug in the original code poliastro/poliastro#902 <poliastro/poliastro#902> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#115 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFZCK6TIG5LVALUXSPEGEHDRMWQ3HANCNFSM4G2TJLTQ> .

shreyasbapat · 2020-07-22T20:40:20Z

In case it's relevant for this PR, we fixed a bug in the original code poliastro/poliastro#902

Fixing this

This was referenced Feb 27, 2019

Add Dastcom5 Module astropy/astroquery#1339

Closed

Contribute NEOs code to astroquery poliastro/poliastro#364

Closed

shreyasbapat force-pushed the dastcom5 branch 4 times, most recently from e76477b to d611927 Compare February 28, 2019 13:04

mommermi self-assigned this Feb 28, 2019

mommermi added the data label Feb 28, 2019

mommermi self-requested a review February 28, 2019 16:43

shreyasbapat force-pushed the dastcom5 branch from 2c2fed7 to b11029c Compare March 9, 2019 00:03

mommermi requested changes Mar 28, 2019

View reviewed changes

astrojuanlu mentioned this pull request Mar 29, 2019

[WIP] Remove Neos from poliastro forever poliastro/poliastro#605

Closed

shreyasbapat force-pushed the dastcom5 branch from 00c3a44 to 1dcc8d2 Compare April 9, 2019 22:56

shreyasbapat force-pushed the dastcom5 branch from 95df0bd to 3a38a16 Compare January 4, 2020 11:19

astrojuanlu reviewed Jan 4, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

shreyasbapat force-pushed the dastcom5 branch from 5554d9b to 7d9c0e5 Compare March 31, 2020 05:30

shreyasbapat force-pushed the dastcom5 branch from d8b46f0 to 150e440 Compare March 31, 2020 08:32

anhiga and others added 16 commits July 23, 2020 02:05

Add dastcom module

ae008dc

Fix the module for sbpy

19a15aa

Add tests for Dastcom5 module

33f021f

Fix PEP8 Errors

317e722

Add pandas to requirements

1633b93

Fix show download progress

de206d1

Add Dastcom to utils

005293d

Add a wrapper around dastcom5

779e6c3

Add text about DASTCOM5 to docs

e85d6b0

Skip the doc example in testing

61e2b54

Use Astropy table instead of pandas

1343367

Move dastcom to sbpy.data.utils

605d9c7

Fix rebase artifacts

878a87a

Add a parser function

99fe232

Fix some more rebase artifacts

f0f2b98

MArk remote tests

2d0bf04

shreyasbapat force-pushed the dastcom5 branch from 150e440 to 2d0bf04 Compare July 22, 2020 20:39

astrojuanlu mentioned this pull request Jan 8, 2021

Remove poliastro.neos poliastro/poliastro#1068

Closed

astrojuanlu mentioned this pull request Apr 27, 2022

Possible collaboration with poliastro for orbital element handling #26

Closed

Add Dastcom5 Service #115

Are you sure you want to change the base?

Add Dastcom5 Service #115

Conversation

shreyasbapat commented Feb 27, 2019 • edited Loading

pep8speaks commented Feb 27, 2019 • edited Loading

Comment last updated at 2020-07-22 20:39:45 UTC

astrojuanlu commented Feb 27, 2019

astrojuanlu commented Feb 28, 2019

shreyasbapat commented Feb 28, 2019

mommermi commented Feb 28, 2019

shreyasbapat commented Feb 28, 2019

shreyasbapat commented Mar 8, 2019 • edited Loading

shreyasbapat commented Mar 13, 2019

mommermi commented Mar 18, 2019

mommermi 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

shreyasbapat Apr 11, 2019 • 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

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

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

shreyasbapat commented May 5, 2019

astrojuanlu commented May 5, 2019

astrojuanlu commented May 5, 2019

mommermi commented May 6, 2019

shreyasbapat commented Jun 20, 2019

shreyasbapat commented Jan 4, 2020

mkelley commented Feb 24, 2020

shreyasbapat commented Feb 27, 2020

shreyasbapat commented Mar 31, 2020

mommermi commented Mar 31, 2020

mkelley commented Apr 8, 2020

shreyasbapat commented Apr 8, 2020

astrojuanlu commented Apr 15, 2020 • edited Loading

shreyasbapat commented Apr 15, 2020 via email

shreyasbapat commented Jul 22, 2020

shreyasbapat commented Feb 27, 2019 •

edited

Loading

pep8speaks commented Feb 27, 2019 •

edited

Loading

shreyasbapat commented Mar 8, 2019 •

edited

Loading

shreyasbapat Apr 11, 2019 •

edited

Loading

astrojuanlu commented Apr 15, 2020 •

edited

Loading