[unidown] Possible logic error in GOESClient.

[sudk1896]

Well , I found that for a particular timerange, GOESClient wasn't returning the intended fits file.

from sunpy.time.timerange import TimeRange
from sunpy.net.vso.attrs import Time, Instrument
from sunpy.net.dataretriever.client import QueryResponse
import sunpy.net.dataretriever.sources.goes as goes

LCClient = goes.GOESClient()
qr = LCClient.query(Time('2012/10/4','2012/10/6'),Instrument('goes'))
res = LCClient.get(qr)
dl = res.wait()
print(len(qr)) #Length of query results
print(len(dl)) #no. of downloaded files.

Well, sometimes a particular timerange and not a particular date might have multiple GOES satellite numbers, Think of a timerange which is spread over two different satellite numbers( see _get_goes_sat_num ) the earlier implementation didn't account for that. My implementation does that. I might be wrong here. This would need some testing as well. Basically, the earlier logic was any given timerange by a user would belong to only one GOES sat number. That might no be the case if we select a small timerange spread over the end of one satellite and beginning of the other. Correct me if I'm wrong.

[Cadair]

what is this file doing here?!

[Cadair]

Am I right in thinking that before the GOES client would have only ever downloaded one file? So this PR fixes that and also deals with the satellite number for each one?

[sudk1896]

Yes, Think of it in this way. Say you have a timerange t1-> '1981-01-01', '1983-04-30' and you have a timerange t2-> '1981-01-01', '1984-07-31'. If you check the _get_goes_sat_num function, it should return sat number [1] for t1 and sat number [1, 2] for t2. But it won't according to its earlier logic.

[Cadair]

@sudk1896 cool, with the earlier logic, how many files would it return? It looks to me like it would only return one URL no?

[Cadair]

ping @wafels @nabobalis @DanRyanIrish for review & comments.

[sudk1896]

@Cadair: Yes, it does return one fits file. Well, the only problem with this approach is its simple, on account of its simplicity, very slow. If a user gives a large timerange, it would go through all the dates in between. I don't see any other uncomplicated way of doing this( I do have something in mind, but its way too messy and complicated). Sometimes simple should do it.
@wafels @nabobalis @DanRyanIrish please take a look and comments are welcome.

[Cadair]

@sudk1896 I would not worry about performance, it would have to be a very very large timerange to run into Python loop speed issues.

Can you remove the extra goes.py you added? and add a test to check it does "the right thing"?

[sudk1896]

@Cadair: Thinking about extreme cases. It would have to be very large. I will add an additional test too and remove that extra goes.py file.

[Cadair]

the remaing goes.py is now in the wrong place, it's been moved.

[sudk1896]

@Cadair: How do I fix this ? I'm a bit lost. I don't know how I committed two goes.py files.

[Cadair]

@sudk1896 probably easiest to move it back to the correct folder with git mv goes.py sunpy/net/dataretriever/sources

[sudk1896]

It says bad source. I'm googling this as we speak.

[DanRyanIrish]

Hi guys. Taking a look at this now.

[DanRyanIrish]

Hi guys. This appears to be a needed improvement since time ranges can included data from multiple GOES satellites. It doesn't just have to be because the time range spans a single point where one GOES satellite takes over from another. There are periods where the primary GOES satellite switches back and forth between two satellites. So we definitely need to be able to handle multiple GOES satellite numbers for a single time range.

Can I just ask how the URL list output is used, both previously and now? As @Cadair mentioned it appears the GOES.Client only ever downloaded one file. In that case, only handling a single GOES satellite number was fine. But does this mean that previously, even if you asked for a time period of two or more days, the GOES.Client would only return the first day's data?

This new code appears to return a list of URLs for the entire user-defined time range in which case multiple GOES satellite numbers may be needed. But how is this list used? I don't see changes to any other code where the URL list is used. Does changing this file alone mean that the GOES.Client can now download multiple files for a time range?

[Cadair]

Does changing this file alone mean that the GOES.Client can now download multiple files for a time range?

Yes, I think so. There should be tests added for this as well.

[DanRyanIrish]

OK. Well if this is the case, then once there are tests to show this I'm happy with the change.

[sudk1896]

Someone with the earlier GOESClient logic please test the query that I have mentioned, to confirm the logic error.
@Cadair, @DanRyanIrish , try and test the same query on your systems as well, if possible.

[Cadair]

@sudk1896 test looks good, can you also add a test which is Fido jusst doing a query. (I think it will not need to be marked as online right?) i.e. not actually downloading the data.

Also a test expicitly checking the sat number would be good (especially one that returns more than one sat number).

[sudk1896]

@Cadair: Yep we could just check how many fits file it has to download and not actually download them.
The aforementioned query returns two sat numbers, I will add that as well. No problem.

[DanRyanIrish]

I don't think I'll be able to check it today. I will over the next couple of days if you still need me to by then.

[sudk1896]

@Cadair: Take a look at the test. Fido works fine btw. I made it online, just to be safe, is that a problem ?

[Cadair]

@sudk1896 that test is good, you should also add one which does a search and then compares the returned URLs to known values, and then doesn't do the fetch. That test could then be offline.

[dpshelio]

@sudk1896 - you can close this pull request yourself, as #1747 is implementing this. But, could you add the information and requests done by @DanRyanIrish regarding different GOES sats as an issue (and reference to this PR number)? Thanks

[sudk1896]

#1747 already does this. Closing this PR.