-
-
Notifications
You must be signed in to change notification settings - Fork 393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Dastcom5 Module #1339
Add Dastcom5 Module #1339
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments. I haven't used unittest.mock
, could you explain what that does?
Thanks for reviewing @keflavich !
As in the documentation of unittest. Is it okay to use that? |
I'll give this a review, too but currently being swamped with non astroquery things. Until then I have one bigger comment:
please switch to use pytest rather than unittest. |
astroquery/dastcom5/core.py
Outdated
with zipfile.ZipFile(dastcom5_zip_path) as myzip: | ||
myzip.extractall(self.local_path) | ||
|
||
class _Show_Download_Progress(tqdm): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what other astroquery developers think, but isn't it extremely weird to have a class here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, particularly because we have already implemented this in astroquery.query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it? Okay! Will figure it out somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the class and used the astroquery's default downloading function
astroquery/dastcom5/core.py
Outdated
|
||
print("Downloading datscom5.zip") | ||
with self._show_download_progress(unit='B', unit_scale=True, miniters=1, desc="dastcom5.zip") as t: | ||
urllib.request.urlretrieve( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooh... interesting... I think we have dealt with this before.... hm. ftp
is the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you suggest then? Any previous issue from which I can infer about what's happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 =(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, there is no HTTPS access to this file, no. Too bad...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay! Am I going to ping them or you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
astroquery/dastcom5/core.py
Outdated
with zipfile.ZipFile(dastcom5_zip_path) as myzip: | ||
myzip.extractall(self.local_path) | ||
|
||
class _Show_Download_Progress(tqdm): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, particularly because we have already implemented this in astroquery.query
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
astroquery/query.py
Outdated
@@ -342,6 +345,9 @@ def _download_file(self, url, local_filepath, timeout=None, auth=None, | |||
response.close() | |||
return response | |||
|
|||
def __ftp_download(self, url, local_filepath): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note you used dunder (__) here, but single-underscore on line 253 above. I think single-underscore is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! That's a mistake one my part! It was not visible to me on my screen. Sorry for that! :(
|
||
|
||
def test_read_record(mocker): | ||
mock_np_fromfile = mocker.patch("poliastro.neos.dastcom5.np.fromfile") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to mock this? np.fromfile
should be directly usable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
def test_download_dastcom5_creates_folder(mocker): | ||
mock_request = mocker.patch("astroquery.dastcom5.urllib.request") | ||
mock_isdir = mocker.patch("astroquery.dastcom5.os.path.isdir") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here; why did you mock this and some of the below methods?
Closing this as discussed in the chats: https://astropy.slack.com/archives/C8U8VGQFM/p1549754624034000 |
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