Add Dastcom5 Service

[shreyasbapat]

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

And from a PR in astroquery: https://github.com/astropy/astroquery/pull/1339

I am adding this module here.

cc @Juanlu001

[pep8speaks]

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]

(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]

@shreyasbapat please fix the PEP8 errors.

[shreyasbapat]

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

[mommermi]

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]

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

[shreyasbapat]

I have created the checklist for reference.

• [x] 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?

• [x] 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.

• [x] 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.

• [x] Could you also please add some examples to the docstrings and write a bit of text for docs/sbpy/data.rst?

[shreyasbapat]

@mommermi I have done everything that was asked!

[mommermi]

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

[shreyasbapat]

@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]

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]

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

https://github.com/spacetelescope/synphot_refactor

[mommermi]

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

[shreyasbapat]

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

[shreyasbapat]

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

[astrojuanlu]

UNEXPECTED EXCEPTION: URLError("ftp error: OSError('An attempt was made to connect to the internet by a test that was not marked remote_data. The requested host was: 137.78.251.144',)",)

@shreyasbapat I am not familiar with sbpy test suite but it seems that doctests can't contain remote connections? Or perhaps they have to be decorated somehow?

E FileNotFoundError: [Errno 2] No such file or directory: '/Users/travis/.sbpy/dastcom5.zip'

Seems like the data download is still not working correctly?

[mkelley]

@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]

Sure. I will do so.

[shreyasbapat]

@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]

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]

@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]

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

[astrojuanlu]

In case it's relevant for this PR, we fixed a bug in the original code https://github.com/poliastro/poliastro/issues/902

[shreyasbapat]

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 < [email protected]> wrote:

In case it's relevant for this PR, we fixed a bug in the original code poliastro/poliastro#902 https://github.com/poliastro/poliastro/issues/902

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NASA-Planetary-Science/sbpy/pull/115#issuecomment-614003280, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZCK6TIG5LVALUXSPEGEHDRMWQ3HANCNFSM4G2TJLTQ .

[shreyasbapat]

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

Fixing this