Add Dastcom5 Module by shreyasbapat · Pull Request #1339 · astropy/astroquery

shreyasbapat · 2019-01-17T17:30:11Z

As per the previous discussion in chats and this comment of @mommermi
#1325 (comment) , in this PR #1325

I have added the DASTCOM5 module in this PR. Please let me know if any change is required.

cc @Juanlu001

pep8speaks · 2019-01-17T17:30:14Z

Hello @shreyasbapat! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 07, 2019 at 17:53 Hours UTC

keflavich

Just a few minor comments. I haven't used unittest.mock, could you explain what that does?

shreyasbapat · 2019-01-29T18:32:45Z

Thanks for reviewing @keflavich !

The patch() decorator / context manager makes it easy to mock classes or objects in a module under test. The object you specify will be replaced with a mock (or other object) during the test and restored when the test ends

As in the documentation of unittest.

Is it okay to use that?

bsipocz · 2019-01-29T18:45:48Z

I'll give this a review, too but currently being swamped with non astroquery things. Until then I have one bigger comment:

As in the documentation of unittest.

please switch to use pytest rather than unittest.

astrojuanlu · 2019-02-05T17:23:49Z

+        with zipfile.ZipFile(dastcom5_zip_path) as myzip:
+            myzip.extractall(self.local_path)
+
+    class _Show_Download_Progress(tqdm):


I don't know what other astroquery developers think, but isn't it extremely weird to have a class here?

Yes, particularly because we have already implemented this in astroquery.query

Is it? Okay! Will figure it out somehow

I removed the class and used the astroquery's default downloading function

keflavich · 2019-02-05T17:51:06Z

+
+            print("Downloading datscom5.zip")
+            with self._show_download_progress(unit='B', unit_scale=True, miniters=1, desc="dastcom5.zip") as t:
+                urllib.request.urlretrieve(


This is not the correct way to retrieve files in astroquery.
(1) We do not use urllib, we use requests
(2) We already have sophisticated downloading tools:
https://github.com/astropy/astroquery/blob/master/astroquery/query.py#L245
Use the _download_file method if you want to download a file.

I am facing errors like this:

>>> dastcom5.Dastcom5().download_dastcom5() Downloading datscom5.zip Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/shreyas/Local Forks/astroquery/astroquery/dastcom5/core.py", line 62, in download_dastcom5 self._download_file(url=ftp_path, local_filepath=dastcom5_zip_path) File "/home/shreyas/Local Forks/astroquery/astroquery/query.py", line 257, in _download_file auth=auth, **kwargs) File "/home/shreyas/Softwares/anaconda3/lib/python3.6/site-packages/requests/sessions.py", line 533, in request resp = self.send(prep, **send_kwargs) File "/home/shreyas/Softwares/anaconda3/lib/python3.6/site-packages/requests/sessions.py", line 640, in send adapter = self.get_adapter(url=request.url) File "/home/shreyas/Softwares/anaconda3/lib/python3.6/site-packages/requests/sessions.py", line 731, in get_adapter raise InvalidSchema("No connection adapters were found for '%s'" % url) requests.exceptions.InvalidSchema: No connection adapters were found for 'ftp://ssd.jpl.nasa.gov/pub/ssd/dastcom5.zip'

oooh... interesting... I think we have dealt with this before.... hm. ftp is the issue.

What do you suggest then? Any previous issue from which I can infer about what's happening?

I don't suppose there's an alternative http site that hosts this?

If not, then we might need to either use the hackey requests-ftp (which I think we should avoid...) or revert to urllib =(

As far as I understand, there is no HTTPS access to this file, no. Too bad...

Okay! Am I going to ping them or you?

Alright, the best suggestion I got was to fall back on urllib. But, instead of doing that here, let's do it in query.py in _download_file. i.e., in that function, have a test for if url[:3] == 'ftp': return self._ftp_download(file) and use urllib to download by FTP

Sure, will do that!

keflavich · 2019-02-05T17:51:25Z

+        with zipfile.ZipFile(dastcom5_zip_path) as myzip:
+            myzip.extractall(self.local_path)
+
+    class _Show_Download_Progress(tqdm):


Yes, particularly because we have already implemented this in astroquery.query

keflavich · 2019-02-06T16:52:50Z

-            with self._show_download_progress(unit='B', unit_scale=True, miniters=1, desc="dastcom5.zip") as t:
-                urllib.request.urlretrieve(
-                    self.ftp_url + "dastcom5.zip", filename=dastcom5_zip_path, reporthook=t.update_to)
+            self._download_file(url=ftp_path, local_filepath=dastcom5_zip_path)


Looks good. You could pass on some of the other optional keyword arguments here like timeout, cache, and continuation if you'd like. But this is fine.

I was hoping it'd work fine, but it isn't on my system. I am unable to understand whether it is my internet connection and system or there is something wrong in the code

…into dastcom

keflavich · 2019-02-07T17:47:47Z

        response.close()
        return response

+    def __ftp_download(self, url, local_filepath):


note you used dunder (__) here, but single-underscore on line 253 above. I think single-underscore is better.

Oh! That's a mistake one my part! It was not visible to me on my screen. Sorry for that! :(

keflavich · 2019-02-07T17:49:05Z

+
+
+def test_read_record(mocker):
+    mock_np_fromfile = mocker.patch("poliastro.neos.dastcom5.np.fromfile")


why do you need to mock this? np.fromfile should be directly usable?

I wanted to assert if that function is being called. For that I have used the method provided by mock.
Like this: https://github.com/astropy/astroquery/pull/1339/files#diff-b7d60e112ed25e166e296e2824e9f2a7R75

keflavich · 2019-02-07T17:50:20Z

+
+def test_download_dastcom5_creates_folder(mocker):
+    mock_request = mocker.patch("astroquery.dastcom5.urllib.request")
+    mock_isdir = mocker.patch("astroquery.dastcom5.os.path.isdir")


same here; why did you mock this and some of the below methods?

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

Closing this as discussed in the chats: https://astropy.slack.com/archives/C8U8VGQFM/p1549754624034000
And opening NASA-Planetary-Science/sbpy#115 in sbpy

anhiga and others added 5 commits January 17, 2019 20:25

Add the dastcom5 module from poliastro

6f12bc7

Add the init file

85e6ca0

Add the core package for querying DASTCOM5

8fd6b0e

Create a stack of tables for returning multiple orbits

dcf2fb5

Complete the DASTCOM5 Module, Add tests

6633d0c

shreyasbapat mentioned this pull request Jan 17, 2019

Contribute NEOs code to astroquery poliastro/poliastro#364

Closed

shreyasbapat added 3 commits January 17, 2019 23:02

Fix PEP8 Errors

1f825fd

Minor changes

01898ca

Add Python2 Support

d3f26ff

keflavich reviewed Jan 29, 2019

View reviewed changes

Comment thread astroquery/dastcom5/core.py Outdated

Comment thread astroquery/dastcom5/core.py Outdated

Work on suggestions

8064ab6

keflavich added the New Service label Jan 30, 2019

astrojuanlu mentioned this pull request Feb 3, 2019

Download progress is not shown in DASTCOM5 poliastro/poliastro#542

Closed

amsuhane and others added 3 commits February 5, 2019 22:25

Fix download progress issue

c973e3e

Remove vstack

8d853fd

Fix PEP8 issues!

d34b2fc

astrojuanlu reviewed Feb 5, 2019

View reviewed changes

keflavich requested changes Feb 5, 2019

View reviewed changes

shreyasbapat and others added 9 commits February 6, 2019 22:11

Remove urllib, remove class, add _download_file

b0f3f07

Add the dastcom5 module from poliastro

9d479d6

Add the init file

0388788

Add the core package for querying DASTCOM5

06c422e

Create a stack of tables for returning multiple orbits

6c8cd85

Complete the DASTCOM5 Module, Add tests

8739e0a

Fix PEP8 Errors

b61374b

Minor changes

d3986f3

Add Python2 Support

bc0d1eb

shreyasbapat and others added 5 commits February 6, 2019 22:18

Work on suggestions

614b835

Fix download progress issue

cf28593

Remove vstack

af17efd

Fix PEP8 issues!

e07b55f

Remove urllib, remove class, add _download_file

83c32a0

keflavich reviewed Feb 6, 2019

View reviewed changes

shreyasbapat added 2 commits February 7, 2019 23:03

Add ftp_download method

9be0535

Merge branch 'dastcom' of https://github.com/shreyasbapat/astroquery …

29843a4

…into dastcom

keflavich reviewed Feb 7, 2019

View reviewed changes

Remove _ from download ftp

10d497f

shreyasbapat mentioned this pull request Feb 27, 2019

Add Dastcom5 Service NASA-Planetary-Science/sbpy#115

Open

shreyasbapat closed this Feb 27, 2019



		def test_read_record(mocker):
		mock_np_fromfile = mocker.patch("poliastro.neos.dastcom5.np.fromfile")

Uh oh!

Conversation

shreyasbapat commented Jan 17, 2019

Uh oh!

pep8speaks commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on February 07, 2019 at 17:53 Hours UTC

Uh oh!

keflavich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shreyasbapat commented Jan 29, 2019

Uh oh!

bsipocz commented Jan 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyasbapat Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyasbapat commented Feb 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pep8speaks commented Jan 17, 2019 •

edited

Loading

shreyasbapat Feb 6, 2019 •

edited

Loading