Adding support for query by Solar Distance #137

NucleonGodX · 2024-07-19T16:33:58Z

As the #134 suggested adding support for querying by solar distance. This pull request extends support for another REQUEST method, doQueryFilteredByDistance.

nabobalis · 2024-07-19T16:39:20Z

Looks like there is a file conflict with main

NucleonGodX · 2024-07-19T16:44:26Z

Looks like there is a file conflict with main

yep, I'll fix it.

sunpy_soar/tests/test_sunpy_soar.py

Co-authored-by: Nabil Freij <[email protected]>

hayesla · 2024-07-31T13:38:21Z

thanks for this PR @NucleonGodX !

Some comments to begin with -

I think it would be best to pass astropy units to a.soar.Distance rather than just floats. i.e. a.soar.Distance(0.2*u.AU, 0.3*u.AU), and equivalent distance etc (u.km, u.Mm).
there seems to be an issue if the passed distances are outside the range e.g. :

>>> res = Fido.search(a.Time("2022-03-30 17:00", "2022-03-30 19:00"), 
                  a.Instrument.eui, a.Provider.soar, a.soar.Distance(0.2, 0.9))
---------------------------------------------------------------------------
HTTPError                                 Traceback (most recent call last)
Cell In[11], line 1
----> 1 res = Fido.search(a.Time("2022-03-30 17:00", "2022-03-30 19:00"), 
      2                   a.Instrument.eui, a.Provider.soar, a.soar.Distance(0.2, 0.9))

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

File ~/opt/miniconda3/envs/sunpysoar_dev/lib/python3.11/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 ~/opt/miniconda3/envs/sunpysoar_dev/lib/python3.11/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 ~/opt/miniconda3/envs/sunpysoar_dev/lib/python3.11/site-packages/sunpy/net/fido_factory.py:243, in _create_and(walker, query, factory)
    241 @query_walker.add_creator(attr.AttrAnd)
    242 def _create_and(walker, query, factory):
--> 243     return factory._make_query_to_client(*query.attrs)

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

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

File ~/dev_things/sunpy-soar/sunpy_soar/client.py:189, in SOARClient._do_search(query)
    187 r = requests.get(f"{tap_endpoint}/sync", params=payload)
    188 log.debug(f"Sent query: {r.url}")
--> 189 r.raise_for_status()
    191 try:
    192     response_json = r.json()

File ~/opt/miniconda3/envs/sunpysoar_dev/lib/python3.11/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=doQueryFilteredByDistance&LANG=ADQL&FORMAT=json&QUERY=SELECT+h1.instrument,%20h1.descriptor,%20h1.level,%20h1.begin_time,%20h1.end_time,%20h1.data_item_id,%20h1.filesize,%20h1.filename,%20h1.soop_name,%20h2.detector,%20h2.wavelength,%20h2.dimension_index+FROM+v_sc_data_item%20AS%20h1%20JOIN%20v_eui_sc_fits%20AS%20h2%20USING%20(data_item_oid)+WHERE+h1.begin_time%3E='2022-03-30+17:00:00'+AND+h1.begin_time%3C='2022-03-30+19:00:00'+AND+h2.dimension_index='1'+AND+h1.instrument='EUI'&DISTANCE(0.2,0.9](http://soar.esac.esa.int/soar-sl-tap/tap/sync?REQUEST=doQueryFilteredByDistance&LANG=ADQL&FORMAT=json&QUERY=SELECT+h1.instrument,%20h1.descriptor,%20h1.level,%20h1.begin_time,%20h1.end_time,%20h1.data_item_id,%20h1.filesize,%20h1.filename,%20h1.soop_name,%20h2.detector,%20h2.wavelength,%20h2.dimension_index+FROM+v_sc_data_item%20AS%20h1%20JOIN%20v_eui_sc_fits%20AS%20h2%20USING%20(data_item_oid)+WHERE+h1.begin_time%3E=%272022-03-30+17:00:00%27+AND+h1.begin_time%3C=%272022-03-30+19:00:00%27+AND+h2.dimension_index=%271%27+AND+h1.instrument=%27EUI%27&DISTANCE(0.2,0.9))

hayesla · 2024-07-31T13:43:30Z

so it seems like if you pass anything outside of (0.28, 1), it doesnt like that - so we'd need to raise a warning etc.

Some tests testing this are also needed.

NucleonGodX · 2024-08-03T15:04:06Z

I'll push all the needful changes.

hayesla

This looks good @NucleonGodX !

I think it also needs some tests for which the wrong units are passed to test for the warning raised. Similarly a test for when you build a search outside of (0.28, 1)

hayesla · 2024-08-06T13:23:29Z

sunpy_soar/attrs.py

+        by solar distance without relying on a specific distance column.
+        """
+        if dist_max is None:
+            dist_max = dist_min


does this actually work though - like if you have a range with the same values in both it wont work in the query, right? So we should force people to give a range.

does this actually work though - like if you have a range with the same values in both it wont work in the query, right? So we should force people to give a range.

Yeah, makes sense.

sunpy_soar/attrs.py

hayesla · 2024-08-06T13:43:50Z

examples/solar_distance_query.py

+###############################################################################
+# We shall start with constructing a search query with instrument, level, detector, and distance.
+
+instrument = a.Instrument("METIS")


can you make this EUI instead. And maybe EUI with the product being eui_hrieuv174_image rather than using METIS. Users would be more interested in building this search for with the EUI instrument, and specifically the HRI telescope.

examples/solar_distance_query.py

Co-authored-by: Laura Hayes <[email protected]>

NucleonGodX · 2024-08-07T07:22:40Z

I'll fix the failing test and change the instrument to EUI once I reach home.

hayesla

This is looking great @NucleonGodX!

I dont know what the test fails are exactly, but once they pass good to go

sunpy_soar/attrs.py

hayesla · 2024-08-07T15:18:48Z

sunpy_soar/tests/test_sunpy_soar.py

+    assert res.file_num == 48
+
+
+def test_distance_out_of_bounds_warning(recwarn):


what is the recwarn here for?

Oh, its a pytest fixture provided by the pytest library that allows you to capture warnings emitted during the execution of a test.

NucleonGodX · 2024-08-08T16:18:55Z

@nabobalis I'll push some changes to fix pre-commit, let me know if they are okay.

nabobalis · 2024-08-08T18:34:06Z

sunpy_soar/client.py

@@ -122,7 +122,7 @@ def add_join_to_query(query: list[str], data_table: str, instrument_table: str):
        return where_part, from_part, select_part

    @staticmethod
-    def _construct_payload(query):
+    def _construct_payload(query):  # NOQA: C901


What is C901?

Its about the function being too complex.

Oh that should be added to the global ignore.

sunpy_soar/attrs.py

NucleonGodX added 2 commits July 19, 2024 21:50

distance added for querying

362f303

minor change

c86628c

Merge branch 'main' into dist-query

02bde2b

nabobalis reviewed Jul 19, 2024

View reviewed changes

sunpy_soar/tests/test_sunpy_soar.py Show resolved Hide resolved

nabobalis reviewed Jul 19, 2024

View reviewed changes

sunpy_soar/tests/test_sunpy_soar.py Show resolved Hide resolved

NucleonGodX and others added 4 commits July 19, 2024 22:26

fix conflict

8fcd610

added distance query tests

e141eb0

Update sunpy_soar/tests/test_sunpy_soar.py

d5b4b2a

Co-authored-by: Nabil Freij <[email protected]>

pre-commit fix

de544ae

nabobalis approved these changes Jul 24, 2024

View reviewed changes

Merge branch 'sunpy:main' into dist-query

df18790

example added for distance

9e4da46

hayesla requested changes Aug 6, 2024

View reviewed changes

NucleonGodX and others added 3 commits August 7, 2024 09:50

quantity_input added

db34848

Apply suggestions from code review

7e6a791

Co-authored-by: Laura Hayes <[email protected]>

suggestions added

56f3843

instrument of example changed to eui

0a8bbb6

hayesla requested changes Aug 7, 2024

View reviewed changes

NucleonGodX and others added 3 commits August 7, 2024 22:29

applied suggestions, doc test fix

9ddcf24

Merge branch 'main' into dist-query

19bfb16

pre-commit fix

4200bfb

nabobalis reviewed Aug 8, 2024

View reviewed changes

sunpy_soar/attrs.py Show resolved Hide resolved

updated .ruff

ea83066

nabobalis requested a review from hayesla August 14, 2024 15:36

ebuchlin mentioned this pull request Aug 15, 2024

Add Developer guide for further development. #147

Merged

hayesla approved these changes Aug 18, 2024

View reviewed changes

hayesla merged commit 37d1227 into sunpy:main Aug 18, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for query by Solar Distance #137

Adding support for query by Solar Distance #137

NucleonGodX commented Jul 19, 2024

nabobalis commented Jul 19, 2024

NucleonGodX commented Jul 19, 2024

hayesla commented Jul 31, 2024

hayesla commented Jul 31, 2024

NucleonGodX commented Aug 3, 2024

hayesla left a comment

hayesla Aug 6, 2024

NucleonGodX Aug 7, 2024

hayesla Aug 6, 2024

NucleonGodX commented Aug 7, 2024

hayesla left a comment

hayesla Aug 7, 2024

NucleonGodX Aug 7, 2024

NucleonGodX commented Aug 8, 2024

nabobalis Aug 8, 2024

NucleonGodX Aug 8, 2024

nabobalis Aug 8, 2024

NucleonGodX Aug 9, 2024

		assert res.file_num == 48


		def test_distance_out_of_bounds_warning(recwarn):

Adding support for query by Solar Distance #137

Adding support for query by Solar Distance #137

Conversation

NucleonGodX commented Jul 19, 2024

nabobalis commented Jul 19, 2024

NucleonGodX commented Jul 19, 2024

hayesla commented Jul 31, 2024

hayesla commented Jul 31, 2024

NucleonGodX commented Aug 3, 2024

hayesla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NucleonGodX commented Aug 7, 2024

hayesla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NucleonGodX commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment