sunpy / sunpy-soar

A sunpy plugin for accessing data in the Solar Orbiter Archive (SOAR).
https://docs.sunpy.org/projects/soar/
BSD 2-Clause "Simplified" License
17 stars 11 forks source link

Adding instr attrs and updating product attrs #71

Closed hayesla closed 1 year ago

hayesla commented 1 year ago

PR Description

This PR now registers the instrument attrs provided by the SOAR, addressing #52

I've also updated the _can_handle_query to filter out searches to the SOAR does supply (i.e. #59)

I've also updated the product attrs from the tools/update_data.py.

codecov-commenter commented 1 year ago

Codecov Report

Base: 97.58% // Head: 97.17% // Decreases project coverage by -0.41% :warning:

Coverage data is based on head (146ca1a) compared to base (19db03c). Patch coverage: 95.45% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #71 +/- ## ========================================== - Coverage 97.58% 97.17% -0.41% ========================================== Files 4 4 Lines 207 248 +41 ========================================== + Hits 202 241 +39 - Misses 5 7 +2 ``` | [Impacted Files](https://codecov.io/gh/sunpy/sunpy-soar/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sunpy) | Coverage Δ | | |---|---|---| | [sunpy\_soar/client.py](https://codecov.io/gh/sunpy/sunpy-soar/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sunpy#diff-c3VucHlfc29hci9jbGllbnQucHk=) | `97.93% <88.23%> (-2.07%)` | :arrow_down: | | [sunpy\_soar/attrs.py](https://codecov.io/gh/sunpy/sunpy-soar/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sunpy#diff-c3VucHlfc29hci9hdHRycy5weQ==) | `90.19% <100.00%> (+0.61%)` | :arrow_up: | | [sunpy\_soar/tests/test\_sunpy\_soar.py](https://codecov.io/gh/sunpy/sunpy-soar/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sunpy#diff-c3VucHlfc29hci90ZXN0cy90ZXN0X3N1bnB5X3NvYXIucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sunpy). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sunpy)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

hayesla commented 1 year ago

just for some context:

Now when you do a.Instrument the SOAR instruments are registered:

>>> a.Instrument.epd
 <sunpy.net.attrs.Instrument(EPD: Energetic Particle Detector) object at 0x119800110>

and when you print a.Instruments the SolO instruments are listed and the SOAR client.

Also, this fixed #59 so that if not SOAR queries are passed it doesnt just return an empty result but none e.g.

>>> r = Fido.search(a.Instrument.aia, a.Time('2022-10-7T00:00:00','2022-10-07T00:00:10'))

Results from 1 Provider:

7 Results from the VSOClient:
Source: http://vso.stanford.edu/cgi-bin/search
Total estimated size: 474.522 Mbyte

       Start Time               End Time        Source Instrument    Wavelength    Provider  Physobs  Wavetype Extent Width Extent Length Extent Type   Size                              Info                          
                                                                      Angstrom                                                                         Mibyte                                                           
----------------------- ----------------------- ------ ---------- ---------------- -------- --------- -------- ------------ ------------- ----------- -------- ---------------------------------------------------------
2022-10-07 00:00:00.000 2022-10-07 00:00:01.000    SDO        AIA   335.0 .. 335.0     JSOC intensity   NARROW         4096          4096    FULLDISK 64.64844 AIA level 1, 4096x4096 [2.901 exposure] [100.00 percentd]
2022-10-07 00:00:04.000 2022-10-07 00:00:05.000    SDO        AIA   193.0 .. 193.0     JSOC intensity   NARROW         4096          4096    FULLDISK 64.64844 AIA level 1, 4096x4096 [2.000 exposure] [100.00 percentd]
2022-10-07 00:00:05.000 2022-10-07 00:00:06.000    SDO        AIA   304.0 .. 304.0     JSOC intensity   NARROW         4096          4096    FULLDISK 64.64844 AIA level 1, 4096x4096 [2.901 exposure] [100.00 percentd]
2022-10-07 00:00:05.000 2022-10-07 00:00:06.000    SDO        AIA 4500.0 .. 4500.0     JSOC intensity   NARROW         4096          4096    FULLDISK 64.64844 AIA level 1, 4096x4096 [0.300 exposure] [100.00 percentd]
2022-10-07 00:00:06.000 2022-10-07 00:00:07.000    SDO        AIA   131.0 .. 131.0     JSOC intensity   NARROW         4096          4096    FULLDISK 64.64844 AIA level 1, 4096x4096 [2.901 exposure] [100.00 percentd]
2022-10-07 00:00:09.000 2022-10-07 00:00:10.000    SDO        AIA   171.0 .. 171.0     JSOC intensity   NARROW         4096          4096    FULLDISK 64.64844 AIA level 1, 4096x4096 [2.000 exposure] [100.00 percentd]
2022-10-07 00:00:09.000 2022-10-07 00:00:10.000    SDO        AIA   211.0 .. 211.0     JSOC intensity   NARROW         4096          4096    FULLDISK 64.64844 AIA level 1, 4096x4096 [2.901 exposure] [100.00 percentd]
hayesla commented 1 year ago

I've also now registered "SOAR" as the a.Provider for this client fixing #72

This means that for example, if you only wanted to search the SOAR and not also the VSO you can pass a.Provider.soar e.g.

>>> res = Fido.search(a.Time("2022-04-01 00:00", "2022-04-01 01:00"),  a.Instrument("MAG"), a.Provider("SOAR"))
<sunpy.net.fido_factory.UnifiedResponse object at 0x123948250>
Results from 1 Provider:

13 Results from the SOARClient:

Instrument       Data product      Level        Start time               End time        Filesize
                                                                                          Mbyte  
---------- ----------------------- ----- ----------------------- ----------------------- --------
       MAG          MAG-SRF-NORMAL    L2 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000   11.004
       MAG MAG-RTN-NORMAL-1-MINUTE    L2 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000    0.032
       MAG          MAG-RTN-NORMAL    L2 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000   11.013
       MAG           MAG-SRF-BURST    L2 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000   86.089
       MAG           MAG-RTN-BURST    L2 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000   86.172
       MAG          MAG-OBS-NORMAL    L1 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000    2.756
       MAG          MAG-IBS-NORMAL    L1 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000    2.702
       MAG          MAG-IBS-NORMAL    L0 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000    1.451
       MAG          MAG-OBS-NORMAL    L0 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000    1.486
       MAG           MAG-IBS-BURST    L1 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000   19.065
       MAG           MAG-OBS-BURST    L1 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000  149.313
       MAG           MAG-OBS-BURST    L0 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000   71.327
       MAG           MAG-IBS-BURST    L0 2022-04-01 00:00:00.000 2022-04-02 00:00:00.000    9.326

This is actually maybe important for using MAG as the VSO registers MAG as the magnetometer on STEREO.

hayesla commented 1 year ago

thanks for suggestions @dstansby, I think this is now ready for a review again.

wtbarnes commented 1 year ago

I think this has introduced a bug when trying to specify a provider which is not the SOAR. Presumably this has to do with some unexpected interaction that the SOAR client now has with the Provider keyword. I think there needs to be some additional logic for not returning a result when the requested provider is not the SOAR.

I also checked that this error did not happen prior to this PR so I'm pretty sure this is a result of this new logic around specifying the provider as the SOAR.

Take the following example,

from sunpy.net import Fido, attrs as a
time_range = a.Time("2022-03-28T11:00","2022-03-28T11:01")
# This searches only the VSO as expected
q = Fido.search(time_range & a.Instrument.eui)
print(q)
# This returns the same thing because all results are from the SDAC
q = Fido.search(time_range & a.Instrument.eui & a.Provider.sdac)
print(q)
import sunpy_soar
# This searches the VSO and the SOAR as expected
q = Fido.search(time_range & a.Instrument.eui)
print(q)
# This only returns results from the SOAR as expected using the new provider key provided in this PR
q = Fido.search(time_range & a.Instrument.eui & a.Provider.soar)
print(q)
# This should return only results from the SDAC/VSO but insteads throws an error because this client tries to search using a non-SOAR provider.
Fido.search(time_range & a.Instrument.eui & a.Provider.sdac)

The exception is,

---------------------------------------------------------------------------
HTTPError                                 Traceback (most recent call last)
Cell In[1], line 17
     15 print(q)
     16 # This should return only results from the SDAC/VSO but insteads throws an error because this client tries to search using a non-SOAR provider.
---> 17 Fido.search(time_range & a.Instrument.eui & a.Provider.sdac)

File ~/mambaforge/envs/sunpy-soar-dev/lib/python3.10/site-packages/sunpy/net/fido_factory.py:312, in UnifiedDownloaderFactory.search(self, *query)
    266 """
    267 Query for data in form of multiple parameters.
    268
   (...)
    309 parts individually.
    310 """
    311 query = attr.and_(*query)
--> 312 results = query_walker.create(query, self)
    314 # If we have searched the VSO but no results were returned, but another
    315 # client generated results, we drop the empty VSO results for tidiness.
    316 # This is because the VSO _can_handle_query is very broad because we
    317 # don't know the full list of supported values we can search for (yet).
    318 results = [r for r in results if not isinstance(r, vso.VSOQueryResponseTable) or len(r) > 0]

File ~/mambaforge/envs/sunpy-soar-dev/lib/python3.10/site-packages/sunpy/net/attr.py:613, in AttrWalker.create(self, *args, **kwargs)
    609 def create(self, *args, **kwargs):
    610     """
    611     Call the create function(s) matching the arguments to this method.
    612     """
--> 613     return self.createmm(self, *args, **kwargs)

File ~/mambaforge/envs/sunpy-soar-dev/lib/python3.10/site-packages/sunpy/util/functools.py:18, in seconddispatch.<locals>.wrapper(*args, **kwargs)
     17 def wrapper(*args, **kwargs):
---> 18     return dispatcher.dispatch(args[1].__class__)(*args, **kwargs)

File ~/mambaforge/envs/sunpy-soar-dev/lib/python3.10/site-packages/sunpy/net/fido_factory.py:241, in _create_and(walker, query, factory)
    239 @query_walker.add_creator(attr.AttrAnd)
    240 def _create_and(walker, query, factory):
--> 241     return factory._make_query_to_client(*query.attrs)

File ~/mambaforge/envs/sunpy-soar-dev/lib/python3.10/site-packages/sunpy/net/fido_factory.py:484, in UnifiedDownloaderFactory._make_query_to_client(self, *query)
    482     if isinstance(tmpclient, vso.VSOClient):
    483         kwargs = dict(response_format="table")
--> 484     results.append(tmpclient.search(*query, **kwargs))
    486 # This method is called by `search` and the results are fed into a
    487 # UnifiedResponse object.
    488 return results

File ~/Documents/codes/sunpy-soar/sunpy_soar/client.py:31, in SOARClient.search(self, *query, **kwargs)
     29     if "provider='SOAR'" in query_parameters:
     30         query_parameters.remove("provider='SOAR'")
---> 31     results.append(self._do_search(query_parameters))
     32 table = astropy.table.vstack(results)
     33 qrt = QueryResponseTable(table, client=self)

File ~/Documents/codes/sunpy-soar/sunpy_soar/client.py:90, in SOARClient._do_search(query)
     88 r = requests.get(f'{tap_endpoint}/sync', params=payload)
     89 log.debug(f'Sent query: {r.url}')
---> 90 r.raise_for_status()
     92 # Do some list/dict wrangling
     93 names = [m['name'] for m in r.json()['metadata']]

File ~/mambaforge/envs/sunpy-soar-dev/lib/python3.10/site-packages/requests/models.py:1021, in Response.raise_for_status(self)
   1016     http_error_msg = (
   1017         f"{self.status_code} Server Error: {reason} for url: {self.url}"
   1018     )
   1020 if http_error_msg:
-> 1021     raise HTTPError(http_error_msg, response=self)

HTTPError: 400 Client Error:  for url: http://soar.esac.esa.int/soar-sl-tap/tap/sync?REQUEST=doQuery&LANG=ADQL&FORMAT=json&QUERY=SELECT+*+FROM+v_sc_data_item+WHERE+begin_time%3E='2022-03-28+11:00:00'+AND+begin_time%3C='2022-03-28+11:01:00'+AND+instrument='EUI'+AND+provider='SDAC'
hayesla commented 1 year ago

thanks for catching this @wtbarnes !

I've now fixed this, and have added tests for these cases. should be fixed now!

hayesla commented 1 year ago

thanks @dstansby - updated now and ready to be squashed&merged

hayesla commented 1 year ago

Is there a way to re-run these tests (i think everything was passing before ...) and then it can be merged in

dstansby commented 1 year ago

Hmm, not sure why codecov is unhappy 😢

dstansby commented 1 year ago

I've regenerated the codecov token, maybe that will do something...

dstansby commented 1 year ago

Yay!