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

Unified Downloader #1088

Closed
wants to merge 57 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@gunner272
Contributor

gunner272 commented Jul 3, 2014

Unified Downloader's primary aim is to remove the downloading of light curve files from the Light Curve module.
This is done with help of custom clients written for LC sources.In addition VSO module is also made compatible with this.
The downloader has same query based capability as the vso module.
Unified Downloader has a factory based implementation requiring the client to have validation class method '_can_handle_query'.

PS: This is not pep8 compliant yet in various parts.I am working on that


ToDo:

  • A Rebase onto master.
  • Testing to make sure that this plays nice with EVE and similar where both the VSO and the specialised client return results.
  • Guide documentation on how this affects Lightcurve.
  • Integration with JSOC. (@Cadair)
  • Try and unify the common attributes of UnifiedResponse
  • Test coverage?!
  • Full, detailed code review.
@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Jul 4, 2014

Member

hey @gunner272 generally it looks very good. As you pointed out it needs some PEP8 work, also you need to reformat your docstrings to comply with NumPyDoc standards https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt have a look at other docstrings in SunPy to get the idea.

Member

Cadair commented Jul 4, 2014

hey @gunner272 generally it looks very good. As you pointed out it needs some PEP8 work, also you need to reformat your docstrings to comply with NumPyDoc standards https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt have a look at other docstrings in SunPy to get the idea.

@gunner272

This comment has been minimized.

Show comment
Hide comment
@gunner272

gunner272 Jul 4, 2014

Contributor

@Cadair : These will not be the final docstrings, I have added them just for the time being to explain the code better, I will formalize them surely.

Contributor

gunner272 commented Jul 4, 2014

@Cadair : These will not be the final docstrings, I have added them just for the time being to explain the code better, I will formalize them surely.

@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Jul 4, 2014

Member

@gunner272 That's fine. Just giving you some infos!!

I will have a play with your framework when I get a chance. I think at this point we need to write lots of tests to break it!!

Member

Cadair commented Jul 4, 2014

@gunner272 That's fine. Just giving you some infos!!

I will have a play with your framework when I get a chance. I think at this point we need to write lots of tests to break it!!

@gunner272

This comment has been minimized.

Show comment
Hide comment
@gunner272

gunner272 Jul 19, 2014

Contributor

Initial PEP8 fix of code

Contributor

gunner272 commented Jul 19, 2014

Initial PEP8 fix of code

@dpshelio

This comment has been minimized.

Show comment
Hide comment
@dpshelio

dpshelio Jul 21, 2014

Member

There are few other lines that need to be aligned properly. Rest looks great and the demo was excellent 👍

Member

dpshelio commented Jul 21, 2014

There are few other lines that need to be aligned properly. Rest looks great and the demo was excellent 👍

@ehsteve

This comment has been minimized.

Show comment
Hide comment
@ehsteve

ehsteve Jul 21, 2014

Member

Developer docs should be written to instruct future developers on how to add new sources.

Member

ehsteve commented Jul 21, 2014

Developer docs should be written to instruct future developers on how to add new sources.

@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Jul 21, 2014

Member

@ehsteve good point, @gunner272 you can add that to the module-level docstring somewhere.

Member

Cadair commented Jul 21, 2014

@ehsteve good point, @gunner272 you can add that to the module-level docstring somewhere.

@@ -0,0 +1,148 @@
from sunpy.net.download import Downloader

This comment has been minimized.

@Cadair

Cadair Jul 24, 2014

Member

you should rejig these imports so that they are in 3 blocks

standard library

other packages

sunpy

with a line break in between each block.

@Cadair

Cadair Jul 24, 2014

Member

you should rejig these imports so that they are in 3 blocks

standard library

other packages

sunpy

with a line break in between each block.

super(downloadresponse, self).__init__(lst)
def wait(self):

This comment has been minimized.

@Cadair

Cadair Jul 24, 2014

Member

this needs a docstring.

@Cadair

Cadair Jul 24, 2014

Member

this needs a docstring.

This comment has been minimized.

@gunner272

gunner272 Aug 6, 2014

Contributor

This doesn't seem to leave, I have added the docstring.

@gunner272

gunner272 Aug 6, 2014

Contributor

This doesn't seem to leave, I have added the docstring.

This comment has been minimized.

@Cadair

Cadair Aug 8, 2014

Member

It must be because it is on the line above the docstring.

@Cadair

Cadair Aug 8, 2014

Member

It must be because it is on the line above the docstring.

@@ -0,0 +1,142 @@
from datetime import timedelta

This comment has been minimized.

@Cadair

Cadair Jul 24, 2014

Member

add a docstring to this module explaining what it does and explaining how to write a new client.

@Cadair

Cadair Jul 24, 2014

Member

add a docstring to this module explaining what it does and explaining how to write a new client.

This comment has been minimized.

@gunner272

gunner272 Aug 7, 2014

Contributor

I added it to docstring for class UnifieDownloaderFactory, would @Cadair want me to move it?

@gunner272

gunner272 Aug 7, 2014

Contributor

I added it to docstring for class UnifieDownloaderFactory, would @Cadair want me to move it?

This comment has been minimized.

@Cadair

Cadair Aug 7, 2014

Member

Ok, actually it shouldn't go here. You should document the design and use of the whole module in the sunpy/net/unifieddownloader__init__.py docstring.

@Cadair

Cadair Aug 7, 2014

Member

Ok, actually it shouldn't go here. You should document the design and use of the whole module in the sunpy/net/unifieddownloader__init__.py docstring.

This comment has been minimized.

@gunner272

gunner272 Aug 8, 2014

Contributor

done

@gunner272

gunner272 Aug 8, 2014

Contributor

done

Show outdated Hide outdated sunpy/time/timerange.py
@aringlis

This comment has been minimized.

Show comment
Hide comment
@aringlis

aringlis Jan 7, 2015

Member

@Cadair where are we with this? I get a couple of import errors when I try to use unified downloader. I pulled Rishabh's branch for testing purposes.

  1. ImportError: cannot import name FileDownloader

Looks like a simple fix - FileDownloader appears to have changed name to just Downloader.

  1. AttributeError: type object 'VSOClient' has no attribute '_can_handle_query'

I presume this function is used when deciding which of the possible clients to use to service a UniDown query.

Member

aringlis commented Jan 7, 2015

@Cadair where are we with this? I get a couple of import errors when I try to use unified downloader. I pulled Rishabh's branch for testing purposes.

  1. ImportError: cannot import name FileDownloader

Looks like a simple fix - FileDownloader appears to have changed name to just Downloader.

  1. AttributeError: type object 'VSOClient' has no attribute '_can_handle_query'

I presume this function is used when deciding which of the possible clients to use to service a UniDown query.

@gunner272

This comment has been minimized.

Show comment
Hide comment
@gunner272

gunner272 Jan 7, 2015

Contributor

@aringlis: try now should work

Contributor

gunner272 commented Jan 7, 2015

@aringlis: try now should work

@aringlis

This comment has been minimized.

Show comment
Hide comment
@aringlis

aringlis Jan 7, 2015

Member

Ok, great - those errors I mentioned were fixed!

Member

aringlis commented Jan 7, 2015

Ok, great - those errors I mentioned were fixed!

@aringlis

This comment has been minimized.

Show comment
Hide comment
@aringlis

aringlis Jan 7, 2015

Member

So there's a print bug when trying to display the results of a UnifiedDownloader query that ends up being serviced by the VSO, e.g.

results = UnifiedDownloader.query(attrs.Time("2012/1/1", "2012/1/2"),
                           attrs.Instrument('eve'),attrs.Detector('esp'))
print results
AttributeError: Time instance has no attribute 't1'

I think @gunner272 @wafels you're already aware of this - is there a handy fix?

Member

aringlis commented Jan 7, 2015

So there's a print bug when trying to display the results of a UnifiedDownloader query that ends up being serviced by the VSO, e.g.

results = UnifiedDownloader.query(attrs.Time("2012/1/1", "2012/1/2"),
                           attrs.Instrument('eve'),attrs.Detector('esp'))
print results
AttributeError: Time instance has no attribute 't1'

I think @gunner272 @wafels you're already aware of this - is there a handy fix?

@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Jan 7, 2015

Member

replace .t1 with .start

Member

Cadair commented Jan 7, 2015

replace .t1 with .start

@gunner272

This comment has been minimized.

Show comment
Hide comment
@gunner272

gunner272 Jan 7, 2015

Contributor

@Cadair, that doesnt seem to work
Time attribute creates start and end objects via calling parse_time which was returning a string,
I think we need to change that to TimeRange

Contributor

gunner272 commented Jan 7, 2015

@Cadair, that doesnt seem to work
Time attribute creates start and end objects via calling parse_time which was returning a string,
I think we need to change that to TimeRange

@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Jan 7, 2015

Member

oh, yeah a TimeRange would be better.

Member

Cadair commented Jan 7, 2015

oh, yeah a TimeRange would be better.

@wafels

This comment has been minimized.

Show comment
Hide comment
@wafels

wafels Jan 7, 2015

Member

It seems that the VSO query response Time object does not parse the times
it gets from the VSO. Therefore the time properties of the VSO query
response object are not SunPy times. Is it better to have these times as
SunPy times?

On Wed, Jan 7, 2015 at 2:40 PM, Stuart Mumford notifications@github.com
wrote:

oh, yeah a TimeRange would be better.

Reply to this email directly or view it on GitHub
#1088 (comment).

Member

wafels commented Jan 7, 2015

It seems that the VSO query response Time object does not parse the times
it gets from the VSO. Therefore the time properties of the VSO query
response object are not SunPy times. Is it better to have these times as
SunPy times?

On Wed, Jan 7, 2015 at 2:40 PM, Stuart Mumford notifications@github.com
wrote:

oh, yeah a TimeRange would be better.

Reply to this email directly or view it on GitHub
#1088 (comment).

@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Jan 7, 2015

Member

as long as it dosen't break anything.

Member

Cadair commented Jan 7, 2015

as long as it dosen't break anything.

@aringlis

This comment has been minimized.

Show comment
Hide comment
@aringlis

aringlis Jan 7, 2015

Member

Depending on the client that's actually used (e.g. EVEClient, LYRAClient, VSO), the UnifiedResponse is not necessarily unified! For example, a response generated from the EVEClient has .url attribute, but a response generated from the VSO does not. Also, .time attributes returned from a VSO query are different, as mentioned above.

Do we need to unify this more, and if so, how should we do it?

Member

aringlis commented Jan 7, 2015

Depending on the client that's actually used (e.g. EVEClient, LYRAClient, VSO), the UnifiedResponse is not necessarily unified! For example, a response generated from the EVEClient has .url attribute, but a response generated from the VSO does not. Also, .time attributes returned from a VSO query are different, as mentioned above.

Do we need to unify this more, and if so, how should we do it?

@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Jan 7, 2015

Member

I think there will be some differences, the JSOCClient is quite different for example. The common attributes such as .time should be the same though.

Member

Cadair commented Jan 7, 2015

I think there will be some differences, the JSOCClient is quite different for example. The common attributes such as .time should be the same though.

@Cadair Cadair removed the 0.6 Sprint label Jan 14, 2015

"""
if not isinstance(date, datetime.date):
raise ValueError("This method requires a date")
filename = "lyra_%s000000_lev%d_%s.fits" % (date.strftime('%Y%m%d-'), 2, 'std')

This comment has been minimized.

@dpshelio

dpshelio Jan 29, 2015

Member

This should be updated to the new way of formating strings:

filename = "lyra_{0:%y%m%s}_lev{1}_{2}.fits".format(date, '2', 'std')
@dpshelio

dpshelio Jan 29, 2015

Member

This should be updated to the new way of formating strings:

filename = "lyra_{0:%y%m%s}_lev{1}_{2}.fits".format(date, '2', 'std')

@Cadair Cadair referenced this pull request Feb 23, 2015

Merged

[Branch]: Unified Downloader #1300

8 of 13 tasks complete

@Cadair Cadair closed this Feb 23, 2015

@Cadair

This comment has been minimized.

Show comment
Hide comment
@Cadair

Cadair Feb 23, 2015

Member

I moved this into a feature branch to facilitate everyone working on it for the sprint at the end of next week.

Member

Cadair commented Feb 23, 2015

I moved this into a feature branch to facilitate everyone working on it for the sprint at the end of next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment