search

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 10 forks source link

Using astropy's PyVO to make URL calls. #129

Closed NucleonGodX closed 3 weeks ago

NucleonGodX commented 2 months ago

This PR is a part of GSoC-2024, this PR is tackling the issue(#32 ) that is using astroquery's tap plus to do URL calls.

Fixes #32

ebuchlin commented 1 month ago

Following this comment on the parent issue #32, TapPlus now recommends to use PyVO instead.

Most of line changes in this PR were to make the ADQL queries non-URL-escaped. The transition to PyVO should then not be too difficult as a next step. Shall we do this? (and then change the title of the PR)

nabobalis commented 1 month ago

Most of line changes in this PR were to make the ADQL queries non-URL-escaped. The transition to PyVO should then not be too difficult as a next step. Shall we do this? (and then change the title of the PR)

Yes we should do that.

NucleonGodX commented 1 month ago

Following this comment on the parent issue #32, TapPlus now recommends to use PyVO instead.

Most of line changes in this PR were to make the ADQL queries non-URL-escaped. The transition to PyVO should then not be too difficult as a next step. Shall we do this? (and then change the title of the PR)

Yep sounds good, also I wanted to ask about one thing, by using these external call methods we restrict the "REQUESTS" to be strictly doQuery, as one of the issues raised a few days back suggested to use other REQUESTS methods to query by distance. So maybe for more diverse queries we would need basic soar payload construction? So till now soar has been using only doQuery method but if we are interested in incorporating other request methods, using anything for external calls maybe won't be a great idea, as I have noticed both TapPlus and PyVo internally sets "REQUESTS" to doQuery only. I need your thoughts on this. @ebuchlin @nabobalis @hayesla .

nabobalis commented 1 month ago

If we need to add support for other queries, we should check upstream with pyvo and then implement it there. That is my immediate thought.

NucleonGodX commented 1 month ago

Alright, l'll migrate this PR to using PyVo for now.

ebuchlin commented 1 month ago

So maybe for more diverse queries we would need basic soar payload construction? So till now soar has been using only doQuery method but if we are interested in incorporating other request methods, using anything for external calls maybe won't be a great idea, as I have noticed both TapPlus and PyVo internally sets "REQUESTS" to doQuery only. I need your thoughts on this. @ebuchlin @nabobalis @hayesla .

I don't know, so I've asked in this comment on the solar distance issue #134.

Going back to requests seems to be a step backwards (although the non-URL encoded ADQL query can still be passed to requests, by using the request payload instead of building the full URL by hand).

hayesla commented 3 weeks ago

OK, so we have discussed this on our weekly calls. And I think the decision was just to punt this for the moment? Am I recalling this correctly?

nabobalis commented 3 weeks ago

OK, so we have discussed this on our weekly calls. And I think the decision was just to punt this for the moment? Am I recalling this correctly?

Yes that is correct.

hayesla commented 3 weeks ago

Ok great. I think maybe we should close this PR for the moment, but link it to the original issue. Is that ok with you @NucleonGodX ?

NucleonGodX commented 3 weeks ago

Ok great. I think maybe we should close this PR for the moment, but link it to the original issue. Is that ok with you @NucleonGodX ?

Yep, thats perfectly fine.