-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
Addressed the Issue #848 - Download data and add entries to database from a HEK query #3061
Conversation
…base from a HEK query
Hello @Ayoob7! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-05-10 09:17:19 UTC |
Thanks for the pull request @Ayoob7! I am a bot that checks pull requests for milestones and changelog entries. If you have any questions about what I am saying, please ask!
If there are any issues with this message, please report them here. |
Hi @Ayoob7, thanks for the pull request! Could you write a test for this new code? It would go in |
Hi Nabil Freij,
Sure, I'll write some test cases as well. (I'm just going through the code
base and familiarizing with it) Thanks for pointing in the right direction.
Regards. 🙂
Ayoob
…On Fri, Apr 19, 2019 at 6:24 PM Nabil Freij ***@***.***> wrote:
Hi @Ayoob7 <https://github.com/Ayoob7>, thanks for the pull request!
Could you write a test for this new code? It would go in
database/tests/test_database.py.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3061 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGBE3RNDWO2ZADOQ5ZPKELDPRG6IHANCNFSM4HHD5KPQ>
.
|
Hi @nabobalis , I cant seem to install [parfive] module in my venv to run the test_database.py, when I try to install parfive in my venv, I get the following:
I would be grateful for any Input on the problem thanks :) |
How are you installing parfive? |
(pip install parfive) just as other packages I've installed like astropy
…On 20 Apr 2019 5:52 p.m., "Nabil Freij" ***@***.***> wrote:
How are you installing parfive?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3061 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGBE3RNRS45T23EKM35WRIDPRMDIZANCNFSM4HHD5KPQ>
.
|
That's odd. I'm not aware there is an issue with installing parfive, at lest our CI seems to be ok. I've not seen that error before, I guess there might be something off with your virtual environment but I don't know what. |
Sorted, I can run the tests, I'll be adding my tests shortly. 👍 :) |
Added the test cases. |
Hey @nabobalis , made the changes as we discussed above, (removed the undo and redo methods for the database in the tests), would you be able to review my PR. |
The test fails:
I assume the fixture was not committed to the file? |
query_result : sunpy.net.vso.QueryResponse | ||
A VSO query response that was returned by the ``query`` method of a | ||
:class:`sunpy.net.vso.VSOClient` object. | ||
path , progress |
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 are these?
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.
Sorry for the late reply, I'll go through, we had an internet outage and slowdown in Sri Lanka, these are parameters for the method "download_from_vso_query_result".
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.
That's fine, there is no rush.
If that is the case, we need to expand on these items (adding type and a description) like we have for query_result
.
Co-Authored-By: Ayoob7 <mohamedayoob01@gmail.com>
Co-Authored-By: Ayoob7 <mohamedayoob01@gmail.com>
Co-Authored-By: Ayoob7 <mohamedayoob01@gmail.com>
Co-Authored-By: Ayoob7 <mohamedayoob01@gmail.com>
sunpy/database/database.py
Outdated
""" | ||
translated_query = itertools.chain.from_iterable( | ||
H2VClient().translate_and_query(query_result)) | ||
self.download_from_vso_query_result(translated_query,path=path,progress=progress) |
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.
self.download_from_vso_query_result(translated_query,path=path,progress=progress) | |
self.download_from_vso_query_result(translated_query, path=path, progress=progress) |
Hi @dpshelio , made the changes for code consistency and pushed them. Would you be able to review and ratify this PR ? Thanks a bunch :) |
Hi David,
Thank you working on the next issue as well. I'll keep you posted.
Regards
…On 23 May 2019 5:31 a.m., "David Pérez-Suárez" ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3061?email_source=notifications&email_token=AGBE3RLZHJRHSQNMTKF4CY3PWXNELA5CNFSM4HHD5KP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZOGBGY#pullrequestreview-240935067>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGBE3RLFYWK7YPG4X2KKVJLPWXNELANCNFSM4HHD5KPQ>
.
|
This pull request has been automatically marked as stale because it has not had any activity for the past five months. It will be closed if no further activity occurs. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea! |
This pull request has been automatically closed since there was no activity for a month since it was marked as stale. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea! |
Addressed the Issue #848 - Download data and add entries to database from a HEK query
Description
Fixes #848